GNOME Bugzilla – Bug 750285
add a button to cycle between all available cameras
Last modified: 2020-11-12 07:12:16 UTC
This can happen: some laptops have two cameras, and you could also cameras in external monitors, or standalone webcams. The button should look similar to the 'selfie' mode button commonly seen in phones. We should also avoid the flash when using the front-facing camera, but that needs the devices to be tagged in udev with this information.
What about a swipe/switch interface as it is implemented in EOG to toggle through the detected cameras? This way it is not limited to two devices and should fit nicely into the ui.
Created attachment 360035 [details] mockup (In reply to Matthias Clasen from comment #0) > This can happen: some laptops have two cameras, and you could also cameras > in external monitors, or standalone webcams. The button should look similar > to the 'selfie' mode button commonly seen in phones. It might be worth having a special UI when there's a front and back camera, since you could use an icon like "rotate". Can we detect when there is a front and back camera? Outside of that case it gets a bit tricky from a design perspective - * I don't think that it makes sense to have a "switch camera" icon, and "Switch Camera" is a bit long for a button label. * Icons for internal and external cameras are likely to be difficult to create and will fall down if there's more than one external camera. * Referring to multiple cameras by name isn't going to look great and is likely going to expose long names that aren't great. Perhaps the best we can do is simply refer to each device by a number? See attached mockup.
We cannot reliably detect back/forward facing cameras, without starting down the path of tagging those, so we wouldn't be able to have 2 different interfaces depending on placement. I would have a single "rotate" like button for cycling through cameras when there are multiple ones. It will handle the 2 cameras on one body correctly, and would be simple enough to not be too confusing when the orientation of the cameras isn't back/forward facing. I think a specific "multiple forward facing cameras" UI is a really edge case that doesn't require specific UI. If we don't make that button a toggle one, then we could even keep it for > 2 cameras, as it could be interpreted as "cycle through cameras" just as well as "rotate the camera".
(In reply to Bastien Nocera from comment #3) > We cannot reliably detect back/forward facing cameras, without starting down > the path of tagging those, so we wouldn't be able to have 2 different > interfaces depending on placement. Thanks, that's useful to know. > I would have a single "rotate" like button for cycling through cameras when > there are multiple ones. It will handle the 2 cameras on one body correctly, > and would be simple enough to not be too confusing when the orientation of > the cameras isn't back/forward facing. > > I think a specific "multiple forward facing cameras" UI is a really edge > case that doesn't require specific UI. Laptops with external monitors and external webcam aren't that uncommon, are they? > If we don't make that button a toggle one, then we could even keep it for > > 2 cameras, as it could be interpreted as "cycle through cameras" just as > well as "rotate the camera". A question about that - am I right in saying that there's typically a delay when you switch to a camera?
(In reply to Allan Day from comment #4) > (In reply to Bastien Nocera from comment #3) > > We cannot reliably detect back/forward facing cameras, without starting down > > the path of tagging those, so we wouldn't be able to have 2 different > > interfaces depending on placement. > > Thanks, that's useful to know. > > > I would have a single "rotate" like button for cycling through cameras when > > there are multiple ones. It will handle the 2 cameras on one body correctly, > > and would be simple enough to not be too confusing when the orientation of > > the cameras isn't back/forward facing. > > > > I think a specific "multiple forward facing cameras" UI is a really edge > > case that doesn't require specific UI. > > Laptops with external monitors and external webcam aren't that uncommon, are > they? Not that uncommon, but a weird thing to optimise for. Why number the cameras, as done in your mockup, if they don't relate to any markings. I would probably know that [1] is my laptop's camera because I only have 2 cameras, and it's not the right one that's selected, for example. > > If we don't make that button a toggle one, then we could even keep it for > > > 2 cameras, as it could be interpreted as "cycle through cameras" just as > > well as "rotate the camera". > > A question about that - am I right in saying that there's typically a delay > when you switch to a camera? It really depends on the camera, but it's not instant in most cases, let's say it takes about a second. You can test this out yourself by switching the camera in the preferences, though you might need the inspector to make the preferences dialogue non-modal so you can see what happens in the main window.
Created attachment 360176 [details] [review] symbolic: camera-switch - for switching camera sources ona system with multiple cameras.
Created attachment 360201 [details] updated mockup Here's an updated mockup that uses the new icon that Jakub kindly provided. The idea is that pressing the button would cycle through the available cameras. If there's more than two cameras, it might be interesting to turn them all on and show a preview from each one that the user can pick from. More than two cameras is a relatively uncommon case of course.
Created attachment 360214 [details] [review] Add camera toggle button and mode combobox This is a first patch updating the UI according to the attached mock up. - It replaces the 3 mode buttons with a mode combobox - Adds a switch camera button (currently with a "view-refresh" button) - Switch camera button is only visible if there is more than 1 camera attached. Please review my patch.
And sorry for being too fast, i know that there is no decision yet... but i was too eager.
Created attachment 360723 [details] [review] Add camera toggle button and mode combobox - V2 Update patch with correct icon
Created attachment 360724 [details] Screenshot showing the changes Feedback required: Is this design & patch ok?
Created attachment 360725 [details] Screenshot of ui update
(In reply to Jan-Michael Brummer from comment #12) > Created attachment 360725 [details] > Screenshot of ui update Looks good to me!
Thanks :) Who is responsible to do a review of my patch?
(In reply to Jan-Michael Brummer from comment #14) > Thanks :) Who is responsible to do a review of my patch? The Cheese maintainer is David King.
By the way, I saw this bug because adwaita-icon-theme 3.27.90 got the icon mentioned on this bug.
Thanks for the information, i've already tried to contact both twice by mail in the meantime but got no feedback :(
Still no maintainer feedback, is there an option to opt-in as maintainer?
(In reply to Jan-Michael Brummer from comment #18) > Still no maintainer feedback, is there an option to opt-in as maintainer? Please remind me to look into this after the GNOME 3.28 release, and I'll review it.
Time for a review Bastien?
Review of attachment 360176 [details] [review]: This doesn't apply cleanly anymore.
Review of attachment 360723 [details] [review]: The glade file seems to includes loads of unrelated changes, including property removal, reordering of widgets. It might be worth having a preparatory patch that would open and save the glade file with a newer version of glade first, then make the changes, so as to avoid all the movement in the patch that adds functionality. When using the switch button, the selected camera in the preferences doesn't change, which it would really need to. Ideally, we'd also remember the last used camera across runs, but seeing as this is not already the case with an unmodified cheese, it could be done in a separate patch. ::: data/cheese-main-window.ui @@ +222,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="icon_name">view-refresh-symbolic</property> This needs to be "camera-switch-symbolic" once jimmac has rebased his patch. ::: src/cheese-window.vala @@ +1386,2 @@ thumb_view.button_press_event.connect (on_thumbnail_button_press_event); + Trailing space on that line.
Created attachment 371331 [details] [review] Preparation patch As recommended: Opened and closed glade file with the latest glade to prepare a solid base for the next patch.
Review of attachment 371331 [details] [review]: Yuck, please do not do that - the UI file was hand-written (or at least hand-modified).
Why is that?
(In reply to David King from comment #24) > Review of attachment 371331 [details] [review] [review]: > > Yuck, please do not do that - the UI file was hand-written (or at least > hand-modified). Do you really expect contributors to hand-make the GtkBuilder files? It's far too high a bar IMO. The original patch wasn't readable which is why I asked for a preparatory patch. If the patch must be made by hand, I think it would be best that you either do it, or don't be surprised if you lose this contribution.
(In reply to Bastien Nocera from comment #26) > (In reply to David King from comment #24) > > Review of attachment 371331 [details] [review] [review] [review]: > > > > Yuck, please do not do that - the UI file was hand-written (or at least > > hand-modified). > > Do you really expect contributors to hand-make the GtkBuilder files? I do not expect a patch which solely touches a lot of unrelated lines in the file, and that is why I rejected it. > It's far too high a bar IMO. The original patch wasn't readable which is why > I asked for a preparatory patch. If the patch must be made by hand, I think > it would be best that you either do it, or don't be surprised if you lose > this contribution. I will happily make (or modify) the patch by hand if it is a problem.
(In reply to David King from comment #27) > (In reply to Bastien Nocera from comment #26) > > (In reply to David King from comment #24) > > > Review of attachment 371331 [details] [review] [review] [review] [review]: > > > > > > Yuck, please do not do that - the UI file was hand-written (or at least > > > hand-modified). > > > > Do you really expect contributors to hand-make the GtkBuilder files? > > I do not expect a patch which solely touches a lot of unrelated lines in the > file, and that is why I rejected it. > > > It's far too high a bar IMO. The original patch wasn't readable which is why > > I asked for a preparatory patch. If the patch must be made by hand, I think > > it would be best that you either do it, or don't be surprised if you lose > > this contribution. > > I will happily make (or modify) the patch by hand if it is a problem. Apart from the icon name used, the glade patch is done from Jan-Michael's side. Can you please clean it up so he can use it for his next iteration of the patch?
Review of attachment 360723 [details] [review]: The patch makes 3 separate changes (button to change camera, combo box instead of buttons for the shooting mode and making the shoot button round), so should be split into 3 separate patches. ::: src/cheese-window.vala @@ +1219,3 @@ + * Change media mode between PHOTO, VIDEO and BURST. + */ + public void on_mode_combobox_changed_event () This (and on_switch_camera_clicked_event) are not -event handlers (there are signals on GtkWidget, such as button-press-event), so drop the _event suffix. @@ +1235,3 @@ + break; + default: + break; This should warn (or assert). @@ +1244,3 @@ + public void on_switch_camera_clicked_event () + { + Cheese.CameraDevice device; "current" or "selected" is probably better here. @@ +1246,3 @@ + Cheese.CameraDevice device; + Cheese.CameraDevice next = null; + GLib.PtrArray array; "cameras" is a better name. @@ +1247,3 @@ + Cheese.CameraDevice next = null; + GLib.PtrArray array; + int i; len in GPtrArray is a guint, so this should also be unsigned. @@ +1254,3 @@ + } + + device = camera.get_selected_device(); Leave a blank space before the opening bracket. @@ +1255,3 @@ + + device = camera.get_selected_device(); + if (device == null) Leave a blank line before if() blocks. @@ +1261,3 @@ + + array = camera.get_camera_devices(); + for (i = 0; i < array.len; i++) This for loop should be simplified (or removed) by using g_ptr_array_find(), which is new in GLib 2.54. @@ +1292,3 @@ + * Set switch camera buttons visible state. + */ + public void set_switch_camera_button_state () Is it better to hide the button entirely, or to make it insensitive?
Thank you for your feedback. Wouldn't it be good to fix the ui file first? It shouldn't be that much work with glade and this would help a lot for new contributions. I've found only a small issue with the new glade file: Toggling through the modes is not possible but this would be fixed with my combobox change. Furthermore you didn't answered my question yet: What's the reason to write it by hand instead of using glade?
Created attachment 371586 [details] [review] Add camera switch button Attached splitted patch #1: Add switch mode button
(In reply to Jan-Michael Brummer from comment #30) > Thank you for your feedback. Wouldn't it be good to fix the ui file first? > It shouldn't be that much work with glade and this would help a lot for new > contributions. No, it would simply make it harder to go through the git history. > Furthermore you didn't answered my question yet: What's the reason to write > it by hand instead of using glade? Glade can change unrelated lines in the file, making it harder to follow history. Also, at the time when work was done on using GtkWidget templates in Cheese, Glade did not support them, so hand-editing was the only option.
Review of attachment 371586 [details] [review]: Most of the comments from my previous review still apply, so please revise the patch again.
Created attachment 371626 [details] [review] Add camera switch button - V2 I've changed the function name according to your review (i missed this one for on_switch_camera_clicked_event. As this is 1/3 of the original patch not all review finding apply here. Two findings are not addressed: - Replacement of the loop with g_ptr_array_find() as i do not have the development version of glib. - set_switch_camera_button_state () should stay like this. There is no reason to show the button to a user with only one camera.
Is this one ok? Any review feedback?
Review of attachment 371626 [details] [review]: ::: src/cheese-window.vala @@ +1238,3 @@ + + cameras = camera.get_camera_devices (); + for (i = 0; i < cameras.len; i++) Leave a blank line before the for loop. @@ +1242,3 @@ + next = (Cheese.CameraDevice )cameras.index (i); + + if (next == selected) This condition makes it pretty clear that the following code should be done outside the loop, so change the loop to only find the current item in the array, and then do all the other work outside the loop. As a bonus, that makes it easier to replace with g_ptr_array_find() later on. @@ +1272,3 @@ + { + Cheese.CameraDevice selected; + GLib.PtrArray array; Call it "cameras" instead.
Created attachment 371939 [details] [review] Add camera switch button - V3 Thanks, i've updated the patch.
Review of attachment 371939 [details] [review]: ::: data/cheese-main-window.ui @@ -41,3 @@ <property name="orientation">horizontal</property> <property name="halign">start</property> - <property name="hexpand">True</property> Was it intentional to drop hexpand here?
Yes, to keep it within the current layout.
(In reply to Jan-Michael Brummer from comment #39) > Yes, to keep it within the current layout. It does not do that with only a single camera, so you will need to come up with a better solution.
Created attachment 371943 [details] [review] Add camera switch button - V4 Ok, i did a quick change to the glade file to expand take action button only. This works fine on every camera setup.
Is this one good to go with? Any other changes requested?
Review of attachment 371943 [details] [review]: Thanks for the patch, I pushed it to master as commit b1dccccafedd4025650a32aa8d915dc3415d5121.
Thanks, do you want separate tickets for the other parts or should i add them here as well?
(In reply to Jan-Michael Brummer from comment #44) > Thanks, do you want separate tickets for the other parts or should i add > them here as well? Here is probably fine.
Created attachment 372282 [details] [review] Add circular style to take action button Ok, here is the next one: Circular button.
Review of attachment 372282 [details] [review]: No thanks.
Created attachment 372426 [details] [review] Replace mode toggle buttons with combobox Based on the cheese mockups replace toggle buttons with a combobox.
(In reply to David King from comment #47) > Review of attachment 372282 [details] [review] [review]: > > No thanks. It would have been nice to reject this one in the first run above instead of requesting 3 separated patches. Is there a special reason for rejected this patch? Based on research rounded buttons draws user attention to this particular point. I guess this is wanted in this place and might be the reason why it is mocked this way.
Review of attachment 372426 [details] [review]: ::: data/cheese-main-window.ui @@ +42,2 @@ <property name="visible">True</property> + <property name="can_focus">False</property> This is the default, so I do not think that there is much use in adding it explicitly. ::: src/cheese-window.vala @@ +1227,3 @@ + { + case 0: + set_current_mode (MediaMode.PHOTO); This is not the same as activating the "mode" action, as the buttons used to do; why not? Additionally, the action state is not tracked, so updating the action does not update the combo box (and disabling the action also does not disable the combo box).
Created attachment 372556 [details] [review] Replace mode toggle buttons with combobox - V2 Updates within this version: - Mode (sensitivness) of combobox and switch-camera button is updated based on selection - Moved combobox callback to application to call the preferences function
Time for a review David?
bugzilla.gnome.org is being replaced by gitlab.gnome.org. We are closing all old bug reports and feature requests in GNOME Bugzilla which have not seen updates for a long time. If you still use cheese and if you still see this bug / want this feature in a recent and currently supported version, then please feel free to report it at https://gitlab.gnome.org/GNOME/cheese/-/issues/ Thank you for creating this report and we are sorry it could not be implemented (volunteer workforce and time is quite limited).