On Thu, 2011-07-14 at 14:26 +0200, Martin Nordholts wrote:

Hi Enrico,

I've made a first review-round of some of your new code. It's not a complete review, but it's a start. I hope the to-the-point comments are OK, I don't mean to be rude.

Note that I'm CCing gimp-developer to keep our correspondence public.

I've done the review by diffing origin/master with origin/soc-2011-gimpunitentry (after locally merging master to your branch) and put the result in a text file, then adding comments inline. The patch has been indented 8 spaces to make the comments stand out more. The diff with comments is found here: http://files.chromecode.com/gimp/soc-2011-gimpunitentry-comments-2011-07-04.txt

If you have comments on my comments, just continue this email thread.

I have an additional comment:

All these changes:

- GtkWidget *size_se; + GimpUnitEntryTable *size_se;

The variable must stay a GtkWidget, it a convention for all widgets, since the widget_new() functions also by convention return a GtkWidget.

--Mitch

Hi,

GimpUnitEntryTable is not a GtkWidget, hence the change. I know, the name is not very good, but we're working on it ;) GimpUnitEntryTable derives from GObject and just holds, among other things, a GtkTable with the entries. So should I change these variables to GObject?

Regards,
Enrico

Michael Natterer schrieb:

I have an additional comment:

All these changes:

- GtkWidget *size_se; + GimpUnitEntryTable *size_se;

The variable must stay a GtkWidget, it a convention for all widgets, since the widget_new() functions also by convention return a GtkWidget.

--Mitch

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

Regards, Enrico

On Thu, 2011-07-14 at 14:26 +0200, Martin Nordholts wrote:

Hi Enrico,

I've made a first review-round of some of your new code. It's not a complete review, but it's a start. I hope the to-the-point comments are OK, I don't mean to be rude.

Note that I'm CCing gimp-developer to keep our correspondence public.

I've done the review by diffing origin/master with origin/soc-2011-gimpunitentry (after locally merging master to your branch) and put the result in a text file, then adding comments inline. The patch has been indented 8 spaces to make the comments stand out more. The diff with comments is found here: http://files.chromecode.com/gimp/soc-2011-gimpunitentry-comments-2011-07-04.txt

If you have comments on my comments, just continue this email thread.

Hi Martin!

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

2011/7/14 Enrico Schröder :

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

And now, ladies and gentleman, I present the same mail in hopefully human-readable formatting:

Hi Martin!

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

2011/7/14 Enrico Schröder :

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

Hi Martin

I implemented your suggestion to follow the GtkSpinButton pattern: The UnitAdjustment now is to be created manually by the client and then set via gimp_unit_entry_set_adjustment(). I removed the majority of functions in GimpUnitEntry which mirrored GimpUnitAdjustment. Only set/get_pixels and set/get_bounds is remaining. I also added gimp_unit_entries_get_adjustment() in addition to gimp_unit_entries_get_entry() so that clients can access the adjustment easily. Have a look at the new API and tell me what you think (especially what part of GimpUnitAdjustment we still should mirror in GimpUnitEntry for convenience). I think I like it better now ;) But I made it a separate commit so we could rewind painlessly...

Best regards Enrico

Am 30.07.2011 um 13:30 schrieb Martin Nordholts:

Hi

Here are my review comments from a rather detailed review round. I've looked carefully at the GimpUnitAdjustment and GimpUnitEntry APIs, as I believe we can get the API in a state good enough for inclusion in the GIMP 2.10 plug-in API (that will also survive into GIMP 3.0). GimpUnitEntries should still be kept around, but not as part of a backwards compatible public API, only as a private convenience for us. Same goes for your new gimp_prop_*()-functions and GimpUnitParser. I did't look very closely at the GimpUnitEntries implementation or API this time. I didn't look very close at GimpUnitParser either since it's a small internal helper which is always nice.

The diff with comments can be found here: http://files.chromecode.com/gimp/gimp-soc-2011-gimpunitentry-review-2011-07-17.txt

Your focus now should be on fixing the review comments (and coding style and documentation) rather than continuing to port GIMP app to use GimpUnitEntry as there is not much time left in the project.

Keep up the good job :)

/ Martin

--

My GIMP Blog: http://www.chromecode.com/
"GIMP 2.8 schedule on tasktaste.com"