Re: [Gimp-developer] GSoC GimpUnitEntry: Review round 1
2011/7/16 Enrico Schröder enni.schroe...@gmail.com: 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? Put them in gimpunitentries.h for now. Later we will move gimpunitentries away from libgimpwidgets anyway until we have a GimpUnitEntries interface we believe in. 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. Ok let's keep it, but don't add such a label where there wasn't one (like in the New Layer Dialog). 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. Sounds good. I would like to remind again though about that our main focus right now is to get a basic GimpUnitEntry to work and integrate it in GIMP, so unless you don't have other things to do, don't work on error indication. 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? Don't know yet :) Let me get back on that. / 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
Re: [Gimp-developer] GSoC GimpUnitEntry: Review round 1
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, 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@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] GSoC GimpUnitEntry: Review round 1
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 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, 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@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] GSoC GimpUnitEntry: Review round 1
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
Re: [Gimp-developer] GSoC GimpUnitEntry: Review round 1
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 ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] GSoC GimpUnitEntry: Review round 1
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 ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] GSoC GimpUnitEntry: Review round 1
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. ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
