Skip to content
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

Add option to switch cameras #134

Merged
merged 9 commits into from
Dec 8, 2020
Merged

Conversation

igordsm
Copy link
Contributor

@igordsm igordsm commented Sep 6, 2020

This PR closes #41 and closes #59 . It adds a dropdown menu with possible cameras and switches to newly plugged ones on the fly. If more than one camera is present the last one provided by Gst.DeviceMonitor is used.

camera-switch-small

src/MainWindow.vala Outdated Show resolved Hide resolved
@cassidyjames cassidyjames requested a review from a team September 8, 2020 16:53
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thank you for working on this feature. This looks really great! I left a few inline comments to clean things up a bit.

One big thing I noticed is that it seems you included a workaround for menuitem labels that shouldn't be necessary with the latest revision of the stylesheet in Odin. So that should save quite a lot of code to not apply this

I think one thing to consider is that we should probably only show the menu button if there is actually more than 1 camera available. It would probably look nice to put this in a Gtk.Revealer with a slide transition

src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
src/Widgets/HeaderBar.vala Outdated Show resolved Hide resolved
@igordsm
Copy link
Contributor Author

igordsm commented Sep 8, 2020

@danrabbit @cassidyjames Thank you for the kind reviews. I'll work on them on the next days. After cleaning up I'll try implementing your suggestion regarding revealing the menu only when more than one camera exists.

@igordsm
Copy link
Contributor Author

igordsm commented Sep 12, 2020

@danrabbit I added some animations with the Gtk.Revealer. The revealing animation is not smooth at all, but I'm running from an USB drive so I might be having some performance issues. It looks like this:

pr-elementary-3

@igordsm
Copy link
Contributor Author

igordsm commented Sep 19, 2020

I have removed the commented section and feel this should be ready for another review.

@cassidyjames cassidyjames requested a review from a team October 12, 2020 20:48
@danirabbit
Copy link
Member

@igordsm can you resolve conflicts between this branch and master?

@igordsm
Copy link
Contributor Author

igordsm commented Nov 12, 2020

@danrabbit Done.

Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UX-wise this is super close for me! Would it be possible to add a RadioButton to the start of the menu items to show which camera is chosen?

@igordsm
Copy link
Contributor Author

igordsm commented Nov 29, 2020

@cassidyjames Done.

camera-switch-radio

Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great for me. Awesome work!

@danirabbit danirabbit merged commit a88cae0 into elementary:master Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add configuration option for switching between multiple webcams Switch to USB webcam if connected
3 participants