-
Notifications
You must be signed in to change notification settings - Fork 663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow camera settings edit in single view. #2427
Conversation
@zagrim I didn't have a closer look at how cam settings behave when we merged this. Will do a test/comparison the next days. |
Co-authored-by: MichaIng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ok to me. I never thought of the settings pane when doing the view switching changes, so nice to see a fix for that.
I noticed a small logical inconsistency (which has been there already before):
When in full-screen mode and clicking on config button in the camera frame, full-screen mode is exited (there's an explicit call to doExitFullScreenCamera
in openSettings
). However, it is still possible to have the settings pane open while full-screen by having the settings pane open when entering full-screen mode. And then, of course, there's no Apply button since the top-bar is not visible. So I think we should add a call to closeSettings
to doFullScreenCamera
.
Agreed, either we consequently exit full screen view when opening settings and hide the bar when entering full screen, or we show bar + settings panel while staying in full screen mode, when hitting settings button, hiding both when closing settings. The latter sounds difficult to implement, so adding |
I'd do it here unless @tunkio objects, since it is related to the general topic. Myself I'd add the call before these lines but that's just a proposal:
|
I don't object at all. This is the right place to do it. You can push the fix if you have time. I don't have time to do it right now. Long day ahead. Of course I can check and test it later. |
I added the call, and did a quick test. |
Tested it, seems to work properly. Thanks! |
Works well here as well. Btw, what do you think about making the settings button a show/hide button, i.e. that settings are hidden when open? Now it is possible to close them with the top left button, but I caught myself trying this with the settings button within the camera frame 😉. At best the tooltip should then be adjusted as well, but not mandatory, IMO. |
No rush with this so it's possible to fix these usability things now. I'm bit busy next couple days but can give moment into this later! |
Yes, definitely, let's get this merged. Created an own issue about the settings button: #2435 |
Enhancement to allow camera settings edit in single camera view. Previously the apply button didn't appear even when settings were changed.