GNOME Bugzilla – Bug 763283
[PATCH] Selection/Flood: improve feature naming in UI and PDB
Last modified: 2016-04-16 07:51:40 UTC
Created attachment 323341 [details] [review] UI patch As discussed some weeks ago on IRC the UI items and PDB entry for the new Selection/Flood operation are not meaningful. It can also be easily confused with the existing Selection/Float. The first attached patch renames the UI menu item and the undo history item to „Remove holes“. The second patch changes the PDB function name and description accordingly. I’m not sure whether it would be useful to also rename the internal function and class names so I first contribute my patches here and ask you to review.
Created attachment 323349 [details] [review] PDB patch Rename PDB function name and description to make it more meaningful and comply with the UI item's name.
You have my blessing to change the name, if that's what's decided (I should hang around on IRC more... :P) I went with "flood" because it's both short and idiomatic (and not *too* surprising, I think), and accurately captures the semantics of the operation (as described in the original bug). While for binary (black and white) selections there's little wiggle room, defining exactly what "holes" are, and how they are "removed", is less obvious for general, multi-level selections. Since most users probably don't care much about that, a clearer name might very well be due. However, I'd err on the side of keeping the technical description (minimum of path maxima) around in the PDB description (even if the rest of it is changed). I know... It's confusing and scary, but it is what it is :) Since users of PDB are probably more technical, there's value in describing the exact semantics of an operation (alongside a more "user friendly" explanation), if it's straightforward and stable. And since it's bikeshedding time ;) ... "Remove holes" might connote that something is being detracted from the selection, when in fact it only grows. Maybe simply "fill holes"? (FWIW, it was originally called "fill interior", and I recall "flood inside" and "flood interior" also having been discussed.)
Well, it's not about bikeshedding. I honestly appreciate the great feature you contributed. I was always missing such a thing. However, the name and the PDB description lack of usability for both creative users (with literally no math background) and developers (with a little more math background). Both require a lot of thinking what the developer might have meant. We had a discussion some weeks ago in IRC and ended at 'Remove holes'. That's what users expect and see: removing holes from the selection. I agree that developers are more familiar with technical terms, but even I as a computer scientist had trouble for days figuring out what exactly 'minimum of path maxima' means. I'm proposing the description from my patch, but am open to find a better one with you that is both precise and developer friendly. BTW I tested my patches with the Keyboard shortcut dialog and found out that the action name (selection-flood) is shown in the UI and thus needs to be renamed accordingly, too. So I think I'll rename it all for consistency throughout the code base.
Just to be clear: I'm fine with the changes. The previous comment was just my 2 c. :)
Also, there is no harm in having both user-readable *and* technical docs in the PDB help strings.
I'm ok with it. Ell contributed a very detailed description of the algorithm. Lately in IRC the idea came up to refer to the theoretical foundations/papers or collect them in a single place for reading. Our developer wiki is a good place for it and Pippin proposed a new key/value pair with an URL to the description. Ell, do you agree to extract it from the code and take it into the wiki? Mitch: Do you think it's sufficient to only change the UI, action and PDB strings or would it be better to rename all 'Flood' occurrences from Ell's code?
I'm all for providing more theoretical background. Feel free to use anything from the code! As for renaming, if we're going down the "remove holes" route, then I think it's worth distinguishing between the abstract action of removing holes, and the specific method used to implement it. The method is, for a lack of a better name, "flood" -- we "remove holes" by applying "flood". This is similar to how we "feather" a selection by applying "Gaussian blur". As such, I think everything under app/operations should be kept as "flood" (not to mention that the code and comments make heavy use of the whole "flood" nomenclature :) Everything else is open season. In particular, we might want to change gimp_channel_flood() and friends.
I'm fine with changing labels because "flood" is really the ad-hoc name of the implementation. But until we have a better name (which I doubt we will), we should stick with "flood" for both the internal API (gimp_channel_flood) *and* the external PDB API (gimp-selection-flood).
Created attachment 325460 [details] [review] New description patch Here's an updated patch for the PDB description, containing both user-friendly and technical description. The implementation names are left as is. I also replaced the detailed code comment by a reference to the new Algorithms page in the wiki and added a reference to the GEGL op. Please review. Just a thought for future algorithm descriptions: from the quality of your (Ell's) contributions I suppose you have academic background. So feel free to send your algorithm description as a paper (PDF, Word, Libre/OpenOffice or TeX). I think it's easier to enter formulas and images in a word or TeX processor than trying to write it in ASCII or ASCII art and later converting it to Wiki markup. I can put the paper as PDF then to our wiki. Thank you.
My original PDB description is admittedly pretty awful. Now that we have a more in-depth description on the wiki (thanks for taking the time to do this!), I think we can just drop the technical part and leave the reference to the wiki.
Fixed in master commit f4adb9a078c20ed44655203dcfd49b4dd04f8c21 Author: Sven Claussner <scl@gimp.org> Date: Wed Feb 3 19:25:02 2016 +0000 Rename UI strings After discussion in IRC some weeks ago give the Selection-Flood items the more meaningful name Selection-Remove holes. modified: app/actions/select-actions.c modified: app/core/gimpselection.c app/actions/select-actions.c | 4 ++-- app/core/gimpselection.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) commit 9272da1a2160052550975cd41c8424451c69c623 Author: Sven Claussner <scl@gimp.org> Date: Sun Apr 3 20:47:17 2016 +0000 Flood op: improve descriptions Add more user friendly description to the PDB op. Partially keep the technical description for the PDB op and add a reference to the developer wiki. Use the new "reference" key for the GEGL op. app/operations/gimpoperationflood.c | 368 ++--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- app/pdb/selection-cmds.c | 5 ++-- 2 files changed, 6 insertions(+), 367 deletions(-)
(In reply to Sven Claussner from comment #11) > > commit 9272da1a2160052550975cd41c8424451c69c623 > Author: Sven Claussner <scl@gimp.org> > Date: Sun Apr 3 20:47:17 2016 +0000 > > Flood op: improve descriptions > > Add more user friendly description to the PDB op. > Partially keep the technical description for the PDB op and add a > reference to the developer wiki. > Use the new "reference" key for the GEGL op. > > app/operations/gimpoperationflood.c | 368 > ++--------------------------------------------------------------------------- > ----------------------------------------------------------------------------- > ------------------------------------------------------------------------- > app/pdb/selection-cmds.c | 5 ++-- > 2 files changed, 6 insertions(+), 367 deletions(-) This commit changed the generated file, but not the generator and every time I rebuild gimp it is regenerated from the old version. I think it should be modified also this one: https://git.gnome.org/browse/gimp/tree/tools/pdbgen/pdb/selection.pdb#n427
Indeed, please fix and re-close this bug.
Created attachment 326063 [details] [review] Patch for generator template and generated files Sorry, that was my fault. I overlooked that this file is generated. Here is a new patch. Please review.
(In reply to Sven Claussner from comment #14) > Created attachment 326063 [details] [review] [review] > Patch for generator template and generated files > > Sorry, that was my fault. I overlooked that this file is generated. > Here is a new patch. Please review. Well the patch is not a git format-patch patch. using git apply it says: ../flood-improve-PDB-description.patch:48: trailing whitespace. This procedure removes holes from the selection, that can come from ../flood-improve-PDB-description.patch:49: trailing whitespace. selecting a patchy area with the Fuzzy Select Tool. ../flood-improve-PDB-description.patch:50: trailing whitespace. In technical terms this procedure floods the selection. fatal: corrupt patch at line 54 I configured my git to warn about trailing whitespaces, but in this case they could be necessary. Anyway the procedure should be: just edit the pdb file in the tools/pdbgen/pdb/ directory and run make. After it successfully completed, so that it has generated all files, commit the files changed that are already in git and run the newly built gimp to confirm that the procedure browser show the new help description.
Created attachment 326114 [details] [review] New Patch for generator template and generated files (In reply to Massimo from comment #15) > > Well the patch is not a git format-patch patch. I used Eclipse to create a git compliant patch, but it seems that failed. Here is a new one with git format-patch HEAD^ If it doesn't work lets solve things on the short way by meeting in IRC. I can be there the next days and also read my mail regularly. > > using git apply it says: > > ../flood-improve-PDB-description.patch:48: trailing whitespace. > This procedure removes holes from the selection, that can come from > ../flood-improve-PDB-description.patch:49: trailing whitespace. > selecting a patchy area with the Fuzzy Select Tool. > ../flood-improve-PDB-description.patch:50: trailing whitespace. > In technical terms this procedure floods the selection. > fatal: corrupt patch at line 54 That should be gone now. > Anyway the procedure should be: just edit the pdb file in the > tools/pdbgen/pdb/ directory and run make. After it successfully > completed, so that it has generated all files, commit > the files changed that are already in git and run the newly > built gimp to confirm that the procedure browser show the > new help description. I called pdbgen.pl as written in the README, but the rest is as you requested and it works here. I even cleaned and uninstalled, ran git clean -xdf and rebuild from scratch.
(In reply to Sven Claussner from comment #16) > Created attachment 326114 [details] [review] [review] > New Patch for generator template and generated files > > (In reply to Massimo from comment #15) > > > > > Well the patch is not a git format-patch patch. > > I used Eclipse to create a git compliant patch, but it seems that failed. > Here is a new one with git format-patch HEAD^ > If it doesn't work lets solve things on the short way by meeting in IRC. > I can be there the next days and also read my mail regularly. > > > > > using git apply it says: > > > > ../flood-improve-PDB-description.patch:48: trailing whitespace. > > This procedure removes holes from the selection, that can come from > > ../flood-improve-PDB-description.patch:49: trailing whitespace. > > selecting a patchy area with the Fuzzy Select Tool. > > ../flood-improve-PDB-description.patch:50: trailing whitespace. > > In technical terms this procedure floods the selection. > > fatal: corrupt patch at line 54 > > That should be gone now. > > > Anyway the procedure should be: just edit the pdb file in the > > tools/pdbgen/pdb/ directory and run make. After it successfully > > completed, so that it has generated all files, commit > > the files changed that are already in git and run the newly > > built gimp to confirm that the procedure browser show the > > new help description. > > I called pdbgen.pl as written in the README, but the rest is as you > requested and it works here. I even cleaned and uninstalled, ran git clean > -xdf and rebuild from scratch. Thank you, Sven. Only one doubt as it seems the previous commit intended to edit also the blurb here: https://git.gnome.org/browse/gimp/tree/tools/pdbgen/pdb/selection.pdb#n427 It said: Remove holes from the image's selection instead of: Flood the image's selection which was the old/original version. In short, once you're sure what's best there you can push I tested and the procedure browser shows the modified version
You wouldn't run any scripts manually, you just edit the files in tools/pdbgen/pdb and run make, then you commit all that got regenerated.
OK, I'll do that and add this info to the README where I got this information from.
Fixed in master: commit 0fa6c7f1215c96706d80ecfdf266e1fefaf84725 Author: Sven Claussner <scl@gimp.org> Date: Fri Apr 15 06:25:33 2016 +0000 flood: improve PDB description completely Improve PDB description in generator template and all generated files. app/pdb/selection-cmds.c | 3 +-- libgimp/gimpselection_pdb.c | 10 +++++----- tools/pdbgen/pdb/selection.pdb | 10 +++++----- 3 files changed, 11 insertions(+), 12 deletions(-) commit c5aef386138e484d0780b7cc24d253a49c3c6a6d Author: Sven Claussner <scl@gimp.org> Date: Sat Apr 16 06:09:04 2016 +0000 Make PDBGEN documentation clearer. Tell the developer not to run pdbgen.pl manually, but make. Describe the destdir environment variable. Update the /lib directory to the current /libgimp directory. README_NEW_PDB_PROC: correct spelling. tools/pdbgen/README | 28 +++++++++++++++++++--------- tools/pdbgen/README_NEW_PDB_PROC | 2 +- 2 files changed, 20 insertions(+), 10 deletions(-) Also fixed in gimp-2-8 where the doc wasn't updated yet: commit b1da106ba38a3deb18636fc3cbf305edb8f7bc50 Author: Sven Claussner <scl@gimp.org> Date: Sat Apr 16 06:09:04 2016 +0000 Make PDBGEN documentation clearer. Tell the developer not to run pdbgen.pl manually, but make. Describe the destdir environment variable. Update the /lib directory to the current /libgimp directory. Correct spelling. tools/pdbgen/README | 30 ++++++++++++++++++++---------- tools/pdbgen/README_NEW_PDB_PROC | 2 +- 2 files changed, 21 insertions(+), 11 deletions(-)