GNOME Bugzilla – Bug 117558
[PATCH] CDDB Slave CApplet needs HIGification
Last modified: 2004-12-22 21:47:04 UTC
Patch incoming. regs, Chris
Created attachment 18342 [details] [review] Proposed patch against HEAD.
Changes: - Moved categories out of GtkFrames, pack them into shiny new, HIG-compliant containers. - Made spacing 100% HIG compliant - Category labels now are made bold using Pango markup - Log On Information => Login Information, "Login" is more common - s/.../:/ for radio buttons. This denotes that they are followed by some button-specific option widgets - realigned some label widgets which used to be right-aligned - Added mnemonics to all widgets that had none. Not sure whether the mnemonic letter is the right one in each case, though. regs, Chris
Created attachment 18343 [details] Screenshot illustrating my changes.
One big non-HIG thing remaining here... a dialog shouldn't have Apply and Close buttons, it should either have Apply-Cancel-OK (or just Cancel-OK) if it's not instant apply, or just Close if it is. Would probably also look better if the Update Server List Button didn't span the width of the dialog, but was just a regular size button (probably right-aligned with the control above it). Extra-wide buttons always run the risk of not being immediately recognised as buttons at all. Mnemonics are reasonable, would be nicer if you could use 'r' for Send Real Info but that has a knock-on effect that may or may not make it worthwhile. Might be nicer not to use "F" for "Other FreeDB server" too-- the thing that distinguishes this item from the rest of the group is "Other", so ideally the mnemonic would come from that word. Again that would mean a bit of a reshuffle that may well not be worth the effort though :)
> One big non-HIG thing remaining here... a dialog shouldn't have Apply > and Close buttons, it should either have Apply-Cancel-OK (or just > Cancel-OK) if it's not instant apply, or just Close if it is. Instant apply patch attached to #117573. > Would probably also look better if the Update Server List Button > didn't span the width of the dialog, but was just a regular size > button (probably right-aligned with the control above it). Extra-wide > buttons always run the risk of not being immediately recognised as > buttons at all. Implemented in my new proposal. > Mnemonics are reasonable, would be nicer if you could use 'r' for Send > Real Info but that has a knock-on effect that may or may not make it > worthwhile. Might be nicer not to use "F" for "Other FreeDB server" > too-- the thing that distinguishes this item from the rest of the > group is "Other", so ideally the mnemonic would come from that word. > Again that would mean a bit of a reshuffle that may well not be > worth the effort though :) I tried to implement this, too - I didn't succeed with completely avoiding clashes, though. Therefore we now have two "Ho_stname:" strings. Maybe you know a better mnemonic assignment. regs, Chris
Created attachment 18349 [details] [review] New patch against HEAD. Implemented most of Calum's suggestions.
Created attachment 18350 [details] New screenshot. Compare it with the old to see the mnemonic changes.
Looks good... I can't see any obvious easy solution to the clashing Hostname mnemonics though, so I guess maybe we should just stick to your original ones. As long as icons on buttons are still fashionable, you could maybe add the stock Reload icon to the Update Server List button as well, but personally I wouldn't be too disappointed if you didn't :)
> Looks good... I can't see any obvious easy solution to the clashing > Hostname mnemonics though, so I guess maybe we should just stick to > your original ones. We have no '_Apply' mnemonic anymore (at least as soon as #117573 gets commited), so I reassigned some and - voila - we have unambiguous mnemonics in the whole dialog. > As long as icons on buttons are still fashionable, you could maybe add > the stock Reload icon to the Update Server List button as well, but > personally I wouldn't be too disappointed if you didn't :) Implemented in my new proposal. You may note that the borders around the update server list in-button widget are a bit shitty compared to the dialog buttons but to be honest I wasn't willing to reconstruct the whole GtkButton construction function as it constructs them in a really weeeird way, it modifies the child widget's allocation. Ugly, ugly, hackish, hackish. We definitly need a new GTK+ method which constructs button independent of their child widget's type. regs, Chris
Created attachment 18355 [details] [review] New patch, 3rd edition. Against HEAD - as always :)
Created attachment 18356 [details] That's the way - ah, ah - I like it. New screenshot.
Looks good to me...
Similar patch applied to CVS HEAD.
Thanks for your patch. I won't close that bug, though: Your patch neither includes the update server list button changes, nor does it set reasonable mnemonics (see Calum's comment on mnemonics) or set the row spacing of each category table to a HIG-compliant value. Plus you still have "Log On Information" as category label. Please, read the details carefully before taking such an action. regs, Chris
dennis: "similar patch" ? I assume that means that you and Manny did some HIGification on the same app at more or less the same time ? I saw similar comments which seemed to imply that you worked on the same stuff in some control center bugs. Wouldn't it be better if both of you worked together on this HIGification stuff, especially because it's quite time consuming, and nearly impossible to merge glade diffs ? Feel free to ignore me, this is some random comment ;)
*** Bug 92099 has been marked as a duplicate of this bug. ***
Christian, please update your version of gnome-media. On 2003-07-20, I already committed the changes you spoke about above. The version in CVS does use the "Login Information" label. It does include the 'Update Server List' button. And, the row and column spacing is HIGified (it turns out an old reference to GNOME_PAD was left in the code). The dialog has no mnemonic collisions.