Hi Martin!

Am Freitag, 15. Juli 2011 um 16:17 schrieb Martin Nordholts:

> 2011/7/14 Enrico Schröder <enni.schroe...@gmail.com 
> (mailto: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, 
> > 
> > ?
> > > 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 
> 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,

Gimp-developer mailing list

Reply via email to