Hi Martin! Am Freitag, 15. Juli 2011 um 16:17 schrieb Martin Nordholts: > 2011/7/14 Enrico Schröder <enni.schroeder@xxxxxxxxx (mailto:enni.schroeder@xxxxxxxxx)>: > > 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. > Ok, I will define the defines ;-) Should the common ones be declared in gimpunitentries.h or should each class/file define them themselves when 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. The prototype is gimp_unit_entry_table_add_label (GimpUnitEntryTable *table, GimpUnit unit, const gchar *id1, const gchar *id2) The function automatically adds a preview label to the table, showing the value of the entries with id1 and id2 in the given unit. I noticed that the new image dialog (and a couple of others) were using a kind of preview label (provided by GimpPropWidgets), so I thought it was nice to have for other use-cases as well. Since GimpUnitEntries is a convenience class, I wanted to provide an easy way to create such a preview label. Maybe some plugin could use it someday. Since it's already there and working, why remove it? It doesn't harm anybody, and we don't have to use it in the standard gimp dialogs. If we keep it, we'd need a better name though, e.g. _add_preview_label. > > 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. Makes sense, I will implement that. GimpEevl even provides the position at which an error occurred, don't know how accurate it is though. I will look into maybe providing a little better error indication than just painting everything read. > > 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. I must say separating the entry-management from creating the UI-container sounds a bit cleaner than it is now. But you had your concerns with that, right? > Nice job so far! Thanks :-) Best regards, Enrico _______________________________________________ Gimp-developer mailing list Gimp-developer@xxxxxxxxxxxxxxxxxxxxxx https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
