2011/7/14 Enrico Schröder <enni.schroe...@gmail.com>: > Hi > > I've adressed most of your comments by now. I have a few comments myself > though, which I wrote directly in the file. I marked them with '##' so you > can search for them. > > The file with the comments is to be found here: > http://userpage.fu-berlin.de/enni/soc-2011-gimpunitentry-comments-2011-07-16.txt
Moving discussion out of the text file: > String literals to identify entries is a bit too runtime-ish (still > better than a numeric literal though), would be better to allow the > compiler to tell you when you made a typo. When/if you have time, > perhaps add a > > gimp_unit_entry_table_add_default_entry (table, GIMP_UNIT_ENTRY_TABLE_WIDTH) > > ? >> The problem with that is that you are limited in how to name your >> entries. What if you want to use GimpUnitEntryTable for, say, the >> radius of a circle. Sure you can define additional enums/defines, >> but that yould result in longer code (and we would be back to using >> int for id's, I bet people would just use 1 and 2 instead of >> defining their own constants). If you make a typo, there is a >> g_warning() in place which tells you that that entry doesn't >> exist. But maybe we need to discuss further ;) Let's use the best of both worlds, keep gimp_unit_entries_add_entry() but provide #defines for the common ones: #define GIMP_UNIT_ENTRY_WIDTH "width" ... so that the compiler catches typos. For entry IDs used once, #defines are not needed. > + gimp_unit_entry_table_add_label (options->size_se, GIMP_UNIT_PIXEL, > "width", "height"); > I don't understand what this line does, what label? Maybe there is a > better function name? >> That function-call automatically adds a label below "height" to the >> table, which displays the value of "width" and "height" in pixels >> (see the new layer dialog). Maybe >> gimp_unit_entry_table_add_preview_label would be a better name? I >> figured that it might be a good feature to be able to preview the >> value in pixels while inputting another unit. I can see the label being useful for debugging, but if a user inputs a value in inches, he is likely not interested in the value in pixels. The few that are can temporarily switch to pixels to see. But, since most are not interested, we should not clutter the UI with that info. So IMO the function should be removed. Another thing: Add a timeout on the red background that is shown on invalid input. When typing in normal speed, the intermediate state should not flash "error", becuase no error has been made. For example, if I type "10 in" in normal pace, the entry will flash in red while in the "10 i" state, which is annoying and distracting. I'm going to make another full review of your code now that you've addressed many of the comments, and I will think extra on the fate of GimpUnitEntries, as we discussed on IRC yesterday. Nice job so far! BR, Martin -- My GIMP Blog: http://www.chromecode.com/ "GIMP 2.8 schedule on tasktaste.com" _______________________________________________ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
