GNOME Bugzilla – Bug 454003
Stock button options not correctly sorted
Last modified: 2007-07-31 14:39:41 UTC
For a GtkButton, if I choose the button to be a stock button, and I open the combo box to choose which stock button it is, the list in the combo is not sorted. It appears it's sorted in the english order, but since the labels are translated, it's not sorted anymore :-)
The list is not sorted alphabetically, its simply listed in the order returned by gtk_stock_list_ids(). I dont think its a good idea to sort it alphabetically either, its nice that all the categories seem to show up together (i.e. back,down,forward,up arrows are together - center,fill, left,right page alignment icons are together etc etc.). I'm gonna mark this one wontfix for now, note that the plan will be to adapt the dialog from bug 359640 so that at least we can group together stock icons that are provided by separate catalogs (note the gnome icons that appear at the bottom of the list).
Created attachment 92412 [details] [review] patch that sort the stokitems This patch (which is a first try - and is still a bit ugly) sort the stock items in order that they appear sorted even in foreign languages. It does not sort the stock items as a whole but it sorts prefix by prefix thus GNOME related stock items as still grouped at the end of the list.
Umm, is there any reason for this ? Does the stock list get a different order depending on locale for some reason and this patch is supposed to fix it ? Currently I really dont think that an alphabetical order is superiour to the order the currently appear (i.e. the way they are returned by gtk_stock_list_ids() and probably the order in which they are listed in the gtk+ sources). I'm also a litte weary of spending energy on the stock list if we're going to go with the icon chooser widget approach outlined in my previous comments.
(In reply to comment #3) > Umm, is there any reason for this ? > > Does the stock list get a different order depending on locale for > some reason and this patch is supposed to fix it ? No, and yes... The stock list is in it's english alphabetical order but it gets translated and then it's very hard to find what you need. For example, in french "Save as" is "Enregistrer sous". This means that a french people would like to see this stock item somewhere at the "E" letter. But it does not. The list seems to be totaly mixed up and you can find the "Enregistrer sous" stock item somewhere around the "S" letter !! > Currently I really dont think that an alphabetical order is > superiour to the order the currently appear (i.e. the way > they are returned by gtk_stock_list_ids() and probably the > order in which they are listed in the gtk+ sources). In english this does not bother, in other languages it is indeed a problem. > I'm also a litte weary of spending energy on the stock list > if we're going to go with the icon chooser widget approach > outlined in my previous comments. I understand this, and as this part of the code might disapear, in favour of an icon chooser, it might be viewed as transitional while waiting :)
Created attachment 92596 [details] A comparison image. At left, the normal behaviour (unsorted). At right the result of the patch (sorted). See the GNOME stock items are sorted on their own.
Just want to add a small note: for French people at least, the current way is really annoying. Each time I've tried to find an item in this list, I've had to scan the list at least twice (if not more...).
(In reply to comment #5) > (In reply to comment #3) > > Umm, is there any reason for this ? > > > > Does the stock list get a different order depending on locale for > > some reason and this patch is supposed to fix it ? > > No, and yes... The stock list is in it's english alphabetical > order but it gets translated and then it's very hard to find > what you need. For example, in french "Save as" is > "Enregistrer sous". This means that a french people would > like to see this stock item somewhere at the "E" letter. > But it does not. The list seems to be totaly mixed up and > you can find the "Enregistrer sous" stock item somewhere > around the "S" letter !! The stock list is _not_ in alphabetical order in english As it stands at least some relevent items can be found grouped together, taking the attached image as an example, in the left hand, unsorted list you can find the "Normal size", "Best fit", "Zoom in" and "Zoom out" icons all together - they seem to me to belong together and be part of a logical group, also note that "Forward", "Next", "Pause", "Play", "Previous", "Record", "Rewind" and "Stop" are all together on the left hand list, on the right hand - they are all sorted, which might be desirable if you assume the user knows the stock name of the icon they are looking for before hand. > > Currently I really dont think that an alphabetical order is > > superiour to the order the currently appear (i.e. the way > > they are returned by gtk_stock_list_ids() and probably the > > order in which they are listed in the gtk+ sources). > > In english this does not bother, in other languages it is > indeed a problem. In what language do you use glade ? I use glade in english and I find this stock list very annoying as an interface - we could definitly be doing better - I dont think that preffering an alphabetical order to the natural listed order will help things here. (In reply to comment #7) > Just want to add a small note: for French people at least, the current way is > really annoying. Each time I've tried to find an item in this list, I've had to > scan the list at least twice (if not more...). > Yes and I want to chime in here and also point out that from my POV as an english user, its equally annoying and I have to scan the list alot to find the icon I want - will shuffling the list really make a difference ? My worry is that after making a change like this we'll just end up frustrated that "Pause" can no longer be found directly beside "Play", "Record" and "Rewind", and that the alphabetical sort will look alot more arbitrary than the current sort order.
(In reply to comment #8) > Yes and I want to chime in here and also point out that from > my POV as an english user, its equally annoying and I have to scan > the list alot to find the icon I want - will shuffling the list > really make a difference ? Well, really, there's no good solution: if you know the label, you won't find it with the current way of doing things (but alphabetical order). If you don't know what you're looking for, then the "context" is useful, so the current way might be better. However... (see below) > My worry is that after making a change like this we'll just end up > frustrated that "Pause" can no longer be found directly beside > "Play", "Record" and "Rewind", and that the alphabetical sort will > look alot more arbitrary than the current sort order. I'm not sure the current sort is always good: for example Yes and No are far from each other :-)
(In reply to comment #8) > The stock list is _not_ in alphabetical order in english I didn't noticed this. I thought it was. > As it stands at least some relevent items can be found grouped > together, taking the attached image as an example, in the left hand, > unsorted list you can find the "Normal size", "Best fit", "Zoom in" > and "Zoom out" icons all together - they seem to me to belong > together and be part of a logical group, also note that > "Forward", "Next", "Pause", "Play", "Previous", "Record", "Rewind" > and "Stop" are all together on the left hand list, on the right hand - > they are all sorted, which might be desirable if you assume the > user knows the stock name of the icon they are looking for before hand. [...] > My worry is that after making a change like this we'll just end up > frustrated that "Pause" can no longer be found directly beside > "Play", "Record" and "Rewind", and that the alphabetical sort will > look alot more arbitrary than the current sort order. It seems to me that this is more a GTK concern in fact. May be GTK should have a new "group" parameter that will group together the icons (for instance "Forward", "Next", "Pause", "Play", "Previous", "Record", "Rewind" and "Stop" icons may be grouped together in a "multimedia" group). Then, we can sort the groups one by one... or imagine a fancy Icon Chooser that will display groups of icons :-)
Well now that we're all on the same page, if you guys want locale sorted stock list in that menu then I've got nothing really against it (like Vincent pointed out, both options are kind of bad, alphabetic might be the lesser evil). About your proposed patch: - In glade we prefer using for loops to iterate over lists - Please pay attention to codeing style, curlies go on newlines with no indentation, code body is indented 8 spaces. - Was this patch based on svn ? is the returned array properly null terminated ? - Is it really your opinion that the special "None" item should appear sorted in the list ? or would we be better off keeping the "None" item on top ? - String allocations & Comments: -value.value_nick = stock_id; /* Passing ownership here */ +value.value_nick = g_strdup (gsi->value_nick); /* Passing ownership here */ Hmmm, you're not really passing ownership if you are strduping the string inside new_from_values() and then dupping them again at this point (the old code would assume that the gtk+ stock strings would stay in memory and not bother duplicating them in memory). Its not that bad to have them allocated since this list is only ever created twice, but where are you freeing your GList of GladeStockItems ?
(In reply to comment #11) > Well now that we're all on the same page, if you guys > want locale sorted stock list in that menu then I've got > nothing really against it (like Vincent pointed out, both > options are kind of bad, alphabetic might be the lesser evil). > > About your proposed patch: > - In glade we prefer using for loops to iterate over lists Ok, I modified this. > - Please pay attention to codeing style, curlies go on newlines > with no indentation, code body is indented 8 spaces. Ok. > - Was this patch based on svn ? is the returned array > properly null terminated ? It was based on the 3.3.1 release. The new proposed patch will be based on svn (version 1384 I guess). The returned array is now properly terminated. > - Is it really your opinion that the special "None" item > should appear sorted in the list ? or would we be better > off keeping the "None" item on top ? No, I understood that the "None" item is a special item that is always on top. It is inserted in a sorted list where it is alone ! But, this may allow one to add specific stock item, sorted on top at a later time. Note that this has a small incidence on the code and the overall speed. > - String allocations & Comments: > > -value.value_nick = stock_id; /* Passing ownership here */ > +value.value_nick = g_strdup (gsi->value_nick); /* Passing ownership here */ > > Hmmm, you're not really passing ownership if you are strduping > the string inside new_from_values() and then dupping them again > at this point (the old code would assume that the gtk+ stock strings > would stay in memory and not bother duplicating them in memory). I do not know what this means. I have kept this comment as it was on the original source code. > Its not that bad to have them allocated since this list is only > ever created twice, but where are you freeing your GList of > GladeStockItems ? This was a concern. I fixed it. The lists are beeing freed while the stock items are added to the array. I also removed unnecessary g_str_dup calls.
Created attachment 92736 [details] [review] Revisited patch to sort the GladeStockItem Enumeration This patch based on svn (1384) should now : - use loops to iterate lists - use 8 space indentation and curlies at beginning of a new line - the special "None" stock item appears on top of the list - free some memory allocation that are not needed - terminate correctly the GArray of GEnumValues The patch is still g_strduping the values inserted in the GArray for convenience and code facility (because it's needed when inserting builtin_stock_images). Thus I do not have to make a special case.
Created attachment 92742 [details] [review] [PATCH] Same as the v2 patch (#92736) except that coding style is better
Ok the patch essentially looks alright, I'm just going to adjust the comments wrt string ownership. Your patch actually treats the strings normally instead of assuming ownership of the strings in the stock list (i.e. my old crackpot version was memory economic by just keeping a reference to the original strings in gtk+), so I'll just remove the confusing comments :)