GNOME Bugzilla – Bug 692629
gpk-application "Required packages" and "Dependent packages" buttons are reversed
Last modified: 2013-08-22 12:07:36 UTC
Created attachment 234521 [details] [review] patch Selecting a package, clicking "Required packages" shows the list of packages that depends on selected package. Viceversa, clicking "Dependent packages" shows the list of prerequisite packages needed in order to install the selected package. I attached a patch, swapping button id in the gpk-application.ui file. This because I verified that all the identifiers (callback, functions, up to packagekit/lib/packagekit-glib2 pk_client_get_requires_async, pk_client_get_depends_async) are semantically reversed. Sorry for bad english. This is my first contribution (very very small) but I think is a starting point to do more in future. Thanks to all for Gnome developers: I really appreciate your work!
*** Bug 693044 has been marked as a duplicate of this bug. ***
(In reply to comment #0) > This because I verified that all the identifiers (callback, functions, up to > packagekit/lib/packagekit-glib2 pk_client_get_requires_async, > pk_client_get_depends_async) are semantically reversed. That looks good for a distro patch or for the stable branch. But it sounds like the "correct" fix would be to reverse all of these?
Hi Michael, you're right. But all the identifiers (function names in particular) need to be changed or they'll end up to do something different from what they "promise" in their names. Furthermore they are part of a library (packagekit/lib/packagekit-glib2) so I can't afford to change them because I don't know where they are used. So I ended up to simply swap button ids... if you have a better idea, I will be happy to try to fix in other/smarter way.
Checking the documentation [1], I think gnome-packagekit is just calling the wrong functions. gpk_application_menu_requires_cb should call pk_client_get_depends_async, not pk_client_get_requires_async. gpk_application_menu_depends_cb needs reversed as well. Think of it this way: gpk_application_menu_depends_cb returns the packages that the current package depends on, the "Required Packages" for this package. I admit it is kind of stupidly confusing... I've pinged the PackageKit list to sanity-check this [2]. [1] http://www.packagekit.org/gtk-doc/PkClient.html [2] http://lists.freedesktop.org/archives/packagekit/2013-August/026151.html
(In reply to comment #4) > Checking the documentation [1], I think gnome-packagekit is just calling the > wrong functions. gpk_application_menu_requires_cb should call > pk_client_get_depends_async, not pk_client_get_requires_async. > gpk_application_menu_depends_cb needs reversed as well. > > Think of it this way: gpk_application_menu_depends_cb returns the packages that > the current package depends on, the "Required Packages" for this package. I > admit it is kind of stupidly confusing... I've pinged the PackageKit list to > sanity-check this [2]. > > [1] http://www.packagekit.org/gtk-doc/PkClient.html > [2] http://lists.freedesktop.org/archives/packagekit/2013-August/026151.html Exactly same thoughts here when patching. In other words it need to swap semantics at some point: you suggest button handler (into C code, needing recompiling, installing, testing, etc.), I've modified button handler id into .ui file instead. This seems much simpler to me: modifying button handler isn't "perfect" too because you need to know that, at some point, in the code, identifier's name logic is "inverted". Moreover I thought a simpler patch would have been easier to review and (maybe) accept ... :) If you think it's better and worthwhile, I can provide a new patch switching calls. However I saw your post at PackageKit and I'd like to see if there are some other comments before doing this.
(In reply to comment #5) > If you think it's better and worthwhile, I can provide a new patch switching > calls. Yes, please and thank you. Richard on the list says to go for it. The place to invert the logic is at the barrier between the application and the library, since its semantics are the opposite of ours. The patch will be much larger of course (I guess you'll have to switch the messages as well), but that's preferable to having the buttons trigger different functions than they seem to, which would be quite confusing. Just place comments above those two pk_client_get function calls so that there's no confusion.
From the list: "BTW on Apper I have them as "Required by" and "Depends on"..." I guess that's the obvious/simplest solution and those labels are more clear than what we have anyway. What do you think? (If we go with this solution, we'd use your existing patch for the gnome-3-8 branch only, since we can't change translatable strings in the stable release.)
Created attachment 252683 [details] [review] patch2 swapped calls instead of button ids
Here's the new patch that swaps calls instead button ids. > From the list: "BTW on Apper I have them as "Required by" and "Depends on"..." > > I guess that's the obvious/simplest solution and those labels are more clear > than what we have anyway. What do you think? (If we go with this solution, > we'd use your existing patch for the gnome-3-8 branch only, since we can't > change translatable strings in the stable release.) Up to you choose what you like. I didn't ever tried to propose changing labels exactly because I was concerned about translations. However this also seems fine to me: feel free to choose whatever you prefer. Let me know if this patch is sufficient or I should do something else. Thanks for your time and comments.
Review of attachment 252683 [details] [review]: Good for stable and master, just fix the whitespace before the two comments you added. They need to be one hard tab so as to line up with the code below.
Created attachment 252700 [details] [review] patch3
Ok, fixed leading whitespaces. Thank you.
Awesome. Thanks for the patch!