GNOME Bugzilla – Bug 702264
[PATCH]: various bug-fixes for cheese
Last modified: 2013-09-03 13:19:22 UTC
Created attachment 246808 [details] tarbal with 35 cheese bugfix patches against 3.8 Hi David, I've had doing some cheese bug-fixing and polishing on my todo list for a while now, since there have been some regressions with the vala port and move to gstreamer-1.0 (which both happened after my previous bug-fixing spree). So I'm happy to report I've finally made some time for this. Attached is a tarbal with 35 patches, some cleanup / preparation work for other patches, but mostly bugfixes. I'll let the commit messages of the patches do any further explaining :) Note these are against the 3.8 branch, as I was mostly doing this to get cheese into better shape for Fedora-19. They should be trivial to rebase to master. I plan to do some more work on cheese, but I wanted to get this first set out there now, since I will be offline this weekend. What I plan to do further is: 1) Fix the behaviour of cheese going from no video devices to 1 or more, and visa versa 2) Look into various cheese crashes reported against Fedora 3) Rebase my work on master (if you don't beat me to it). I think we should also consider doing a 3.8.3 release with these bugfixes in there ... Regards, Hans
wow, impressive patch set, Hans.
Thanks for the huge set of patches, Hans! I have started rebasing and improving the patchset in preparation for the 3.9.3 release on Monday (2013-06-17). My current progress on this is in the wip/hans-fixes-master branch, which I will be rebasing as I move commits to it: https://git.gnome.org/browse/cheese/log/?h=wip/hans-fixes-master Most of my modifications are tweaks to the commit messages, reflowing some lines to keep to 79 characters or fewer and other minor issues. I need to look more carefully at the action, FPS and cheese_init() changes, as some of those do not seem immediately mergable to me. I am a bit busy (at the Open Help conference and hackfests until 2013-06-20) but will hopefully get most of your patchset merged to master for 3.9.3. Unfortunately, this patch set is too big and too late in the stable cycle for parts of it to make it to a stable 3.8.3 release, which I was not planning to make in any case. For the future, it is better to target your patches towards master.
(In reply to comment #2) > Thanks for the huge set of patches, Hans! You're welcome. > I have started rebasing and improving the patchset in preparation for the 3.9.3 > release on Monday (2013-06-17). My current progress on this is in the > wip/hans-fixes-master branch, which I will be rebasing as I move commits to it: > > https://git.gnome.org/browse/cheese/log/?h=wip/hans-fixes-master > > Most of my modifications are tweaks to the commit messages, reflowing some > lines to keep to 79 characters or fewer and other minor issues. I need to look > more carefully at the action, FPS and cheese_init() changes, as some of those > do not seem immediately mergable to me. I am a bit busy (at the Open Help > conference and hackfests until 2013-06-20) but will hopefully get most of your > patchset merged to master for 3.9.3. Ok, I've rebased the non merged patches on top of hans-fixes-master, you can find them here: http://cgit.freedesktop.org/~jwrdegoede/cheese/ I'm curious about your objections against the fps and init patches. I assume you'll get into more details about those when you've more time.
I've figured out what one of your concerns with my action enabling / disabling patches may have been, I was disabling only the buttons, leaving the entries in the app-menu still sensitive. I've done a forced push to: http://cgit.freedesktop.org/~jwrdegoede/cheese/ Which has a new version of the action patches which now properly enables/disables the actions rather then the buttons.
Quick update: I've rebased my personal tree on top of the latest master (3.9.3 release) and added a bunch of fixes for starting cheese with no camera and then plugging in a camera (this no works as one would expect it to work). Next on my "hitlist" is making things not fall-apart when unplugging the (last) webcam.
And one more update, I've just done another (forced) push to the master branch of my personal cheese repo. This latest update includes the following changes: -rebased to latest master -cleanly deal with going from 1 -> 0 cameras, as well as going 1 -> 0 -> 1 cameras, cheese behaves as one would expect in all these scenarios now -don't allow changing the resolution and / or the camera device halfway through a recording. This concludes my work on cheese for now, so no further changes will be coming for a while, and this would be a good moment to start merging patches.
Ok, so I ended up adding 2 more bugfixes, but now I'm really really done. One of the 2 fixes a nasty regression caused by the switch to autocluttersink, see: http://cgit.freedesktop.org/~jwrdegoede/cheese/commit/?id=6a1ddb157077bf2db5072077c936599800a1ce52 Note that this one depends on (and partly reverts) this one: http://cgit.freedesktop.org/~jwrdegoede/cheese/commit/?id=77d948d038583cc3c9287dbf035ff32d047c2084 Which was an earlier, incomplete attempt to fix the same issue (it fixed my reproducer, but then I later found another way to reproduce the same issue, and had to "re-design" my fix).
I have pushed a few patches to master, and am slowly digesting the remainder. Bug 698399 was closed thanks to commit ac1852761a30449fb1eb96325800cab34cd6df6c.
I pushed another round of changes, including commit e543a8dec67229370820b1dccca1279e091cdf60 which ensures that the thumbnail view is not shown in fullscreen. I took a different approach with the action and sensitivity handling, by moving more of it into Cheese.Application. Rather than adding the _init_with_args() functions, I made Cheese only handle its own command-line options. I intentionally did not add wide mode to the app menu, as the menu is already rather long and the option will be removed at some point (see bug 668602). The remaining changes are the hotplug and FPS patches, which I will review and merge soon.
(In reply to comment #9) > I pushed another round of changes Thanks for all your work on merging and reviewing my massive patch bomb! > Rather than adding the _init_with_args() functions, I made Cheese only handle its own > command-line options. Ugh, why? I know one of the _init_with_args() functions is somewhat hairy, but that is just because initializing clutter-gtk properly is hairy, ie the empathy code does exactly the same. And all gnome libraries (cheese is a lib), offer _init_with_args() functions, exactly to make it easy for app developers to be able to easily do command-line parsing including library options by offering a _init_with_args(). I really believe that libcheese should offer that too, mimicking all the other gnome-libs. And once we have that doing this properly in cheese itself is easy. Also I did all the hardwork on this, all you need to do is merge it ... > I intentionally did not add wide mode to the app menu, as > the menu is already rather long and the option will be removed at some point > (see bug 668602). Hmm, I've added a comment to bug 668602 on this.
(In reply to comment #10) > (In reply to comment #9) > > Rather than adding the _init_with_args() functions, I made Cheese only handle its own > > command-line options. > > Ugh, why? I know one of the _init_with_args() functions is somewhat hairy, but > that is just > because initializing clutter-gtk properly is hairy, ie the empathy code does > exactly the same. As far as I can see, all of the functionality that the command-line arguments provided is available by setting environment variables, so I see little benefit in adding more API to libcheese and libcheese-gtk to support an alternative method of controlling the underlying libraries. > Also I did all the hardwork on this, all you need to do is merge it ... Thanks for your hard work. I do not merge every patch that is submitted for Cheese, and in this case I have chosen not to merge this commit, and several other commits in your branch.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > Rather than adding the _init_with_args() functions, I made Cheese only handle its own > > > command-line options. > > > > Ugh, why? I know one of the _init_with_args() functions is somewhat hairy, but > > that is just > > because initializing clutter-gtk properly is hairy, ie the empathy code does > > exactly the same. > > As far as I can see, all of the functionality that the command-line arguments > provided is available by setting environment variables, so I see little benefit > in adding more API to libcheese and libcheese-gtk to support an alternative > method of controlling the underlying libraries. Right, and the same goes for any other gnome-application, yet all the other gnome-applications offer these cmdline options too. I thought gnome tried to offer a consistent end user experience over all its components? Also using environment variables requires first googling for them, --help is much more discoverable.
(In reply to comment #12) > (In reply to comment #11) > > As far as I can see, all of the functionality that the command-line arguments > > provided is available by setting environment variables, so I see little benefit > > in adding more API to libcheese and libcheese-gtk to support an alternative > > method of controlling the underlying libraries. > > Right, and the same goes for any other gnome-application, yet all the other > gnome-applications offer these cmdline options too. No, they do not, so I suggest you investigate further before making such claims. > I thought gnome tried to > offer a consistent end user experience over all its components? Using environment variables is the most consistent way to handle this functionality, because it works without any changes to an application (or library, or both). I do not want to add API to libcheese for handling command-line arguments beyond what already exists. > Also using environment variables requires first googling for them, --help is > much more discoverable. I agree that environment variables are less disoverable than command-line arguments. However, the command-line argument help in Cheese was broken since at least 3.6, and there have been no bug reports, so I do not think that discoverability is a problem in this specific case. The Cheese arguments are now discoverable, which is a great improvment on the broken behavour.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > As far as I can see, all of the functionality that the command-line arguments > > > provided is available by setting environment variables, so I see little benefit > > > in adding more API to libcheese and libcheese-gtk to support an alternative > > > method of controlling the underlying libraries. > > > > Right, and the same goes for any other gnome-application, yet all the other > > gnome-applications offer these cmdline options too. > > No, they do not, so I suggest you investigate further before making such > claims. [hans@localhost sunxi-boards]$ totem --help Usage: totem [OPTION...] - Play movies and songs Help Options: -h, --help Show help options --help-all Show all help options --help-gst Show GStreamer Options --help-gtk Show GTK+ Options <snip> [hans@localhost sunxi-boards]$ gnome-terminal --help Usage: gnome-terminal [OPTION...] Help Options: -h, --help Show help options --help-all Show all help options --help-gtk Show GTK+ Options --help-terminal Show terminal options --help-window-options Show per-window options --help-terminal-options Show per-terminal options <snip> [hans@localhost sunxi-boards]$ gedit --help Usage: gedit [OPTION...] [FILE...] [+LINE[:COLUMN]] - Edit text files Help Options: -h, --help Show help options --help-all Show all help options --help-gtk Show GTK+ Options [hans@localhost sunxi-boards]$ gnome-help --help Usage: gnome-help [OPTION...] Help Options: -h, --help Show help options --help-all Show all help options --help-gtk Show GTK+ Options <snip> [hans@localhost sunxi-boards]$ gnome-calculator --help Usage: gnome-calculator [OPTION...] Perform mathematical calculations Help Options: -h, --help Show help options --help-all Show all help options --help-gtk Show GTK+ Options <snip> etc.
You said "all"(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > Right, and the same goes for any other gnome-application, yet all the other > > > gnome-applications offer these cmdline options too. > > > > No, they do not, so I suggest you investigate further before making such > > claims. >… > etc. You wrote "all the other gnome-applications offer these cmdline options too", which is demonstrably false: Documents does not and neither does Boxes. I suspect that if I investigated further then I would find more applications which do not, as well as plenty that do. I do not find your argument convincing, and I have already said that I do not want to add the API to libcheese. I suggest that we move on and discuss the remaining hotplug and FPS patches, if anything comes up during my review.
(In reply to comment #15) > You said "all"(In reply to comment #14) > > (In reply to comment #13) > > > (In reply to comment #12) > > > > Right, and the same goes for any other gnome-application, yet all the other > > > > gnome-applications offer these cmdline options too. > > > > > > No, they do not, so I suggest you investigate further before making such > > > claims. > >… > > etc. > > You wrote "all the other gnome-applications offer these cmdline options too", > which is demonstrably false: Documents does not and neither does Boxes. Ok, so make it almost all, you've found the few exceptions that prove the rule. gnome-documents does not do anything useful with its cmdline arguments at all: [hans@shalem cheese]$ gnome-documents --help Usage: gjs-console [OPTION...] Help Options: -h, --help Show help options Application Options: -c, --command=COMMAND Program passed in as a string -I, --include-path=DIR Add the directory DIR to the list of directories to search for js files. --js-version=JSVERSION JavaScript version (e.g. "default", "1.8" So really is a bad example to use for how to properly do cmdline handling, I've filed an RFE to get this fixed, see bug 703506. As for boxes, like gnome-documents that is one of the newer gnome-apps, and I'm pretty sure the lack of handling of clutter-gtk and spice-gtk cmdline options is an oversight, I've filed bug 703505 for this. > I have already said that I do not want to add the API to libcheese. If that is the problem I will happily rewrite the patch so that proper handling is done inside the vala code without any libcheese changes, would that be acceptable? > I suggest that we move on I'm sorry, but IMHO your solution to the cheese cmdline handling issues is just wrong, it is inconsistent with how the majority of the gnome apps deal with this, and consistency is important. Also most of the gnome libraries offer a foo_get_option_group() for a good reason, so I'll keep pushing for a proper fix for this.
(In reply to comment #16) > (In reply to comment #15) > > I have already said that I do not want to add the API to libcheese. > > If that is the problem I will happily rewrite the patch so that proper handling > is done inside the vala code without any libcheese changes, would that be > acceptable? No, as I have already mentioned that I do not find the consistency argument convincing. The same functionality is avilable by setting environment variables, and I do not wish to add API to libcheese nor alter Cheese to add command-line option handling for underlying libraries. > > I suggest that we move on > > I'm sorry, but IMHO your solution to the cheese cmdline handling issues is just > wrong, it is inconsistent with how the majority of the gnome apps deal with > this, and consistency is important. Also most of the gnome libraries offer a > foo_get_option_group() for a good reason, so I'll keep pushing for a proper fix > for this. I feel that I have made my position clear. You are welcome to disagree, but I will not debate the issue further.
(In reply to comment #17) > No, as I have already mentioned that I do not find the consistency argument > convincing. The same functionality is avilable by setting environment > variables, and I do not wish to add API to libcheese nor alter Cheese to add > command-line option handling for underlying libraries. "I do not wish" is not a technical argument. Technical arguments in favor of adding cmdline support are: 1) It offers a consistent experience to our (cmdline) users 2) It uses the underlying gnome libraries as they are intended to be used. Resorting to creating a fake empty argv array to pass into cheese_gtk_init is an ugly hack, which is necessary because you're actively circumventing the cmdline parsing mechanism which is part of the base gnome platform. What are the *techincal arguments against doing so?
I've pushed a rebased tree with the remaining patches, dropping the contested commandline parsing ones here: http://cgit.freedesktop.org/~jwrdegoede/cheese/log/
Comment on attachment 246808 [details] tarbal with 35 cheese bugfix patches against 3.8 Thanks for rebasing the patch series. I rebased and cleaned up the framerate patches, and they are in Cheese 3.9.91. I do not like the approach with the hotplug changes of calling cheese_camera_setup() in the preferences dialogue, but that is best discussed in bug 603612. The cluttersink change is not correct. The click/tap handler change needs a bug against Clutter if there is a problem. The remaining changes are some state-handling changes which I would ask you to file a separate bug for.
Hi, (In reply to comment #20) > (From update of attachment 246808 [details]) > Thanks for rebasing the patch series. I rebased and cleaned up the framerate > patches, and they are in Cheese 3.9.91. Thanks for merging these! > I do not like the approach with the > hotplug changes of calling cheese_camera_setup() in the preferences dialogue, > but that is best discussed in bug 603612. Ok, I will add a comment to bug 603612 about this. > The cluttersink change is not correct. Ok, thinks seem to work fine without it now, not sure why think were foo-barred for me before. > The click/tap handler change needs a bug against Clutter if there is a problem. This appears to be fixed now (running a fully up2date Fedora-20 pre-alpha). > The remaining changes are some state-handling changes which I would ask you to > file a separate bug for. Ok, I've filed one bug with 2 small bug-fixes in this area, bug 707386, and another bug with a behavior change, bug 707387 . Regards, Hans