Created attachment 70437 [details]
Example screenshot of a problematic case

I am going to disagree with your expected result. I understand your premise but theoretically Select All should create a selection mask that extends out to infinity and a Border operation should not result in a change. 

OK, that's a little esoteric but consider if a selection were to extend beyond the image boundaries as shown in the attachment. If one were to Border this selection (as it was drawn by the user), the edges of the image should not be affected unless the Bordering of the drawn ellipse was large enough to intersect the image boundaries. Your proposal is based on the premise that regions outside the image boundaries are "unselected" whereas I would contend that they are "indeterminant" (and in the case of Select All should really be considered "selected").

Having said that, the actual implementation of a patch to fix your "bug" should be rather trivial (I think; it would only entail temporarily increasing the image size while performing the Border operation and then restoring it) and if the developers were amenable, I would be willing to create such a patch.

I would propose the following:

The PDB remains unchanged and the default behavior should remain as it currently is implemented; it is only two extra PDB calls to implement your desired behavior in a script (this same philosophy was followed in implementing the "Feather border" option to this command earlier this year). 

The GUI for the "Select->Border" command would have a check box added which would cause your desired behavior to be used. I would propose the wording "Shrink from image border" because, though it may not be precise, it should be understandable (as opposed to "Border from image border"). Of course, a better wording may very well be found.

How does this behave for a selection that covers the whole image, but has been created manually?

Wrongly. 

It does not "border" the edges of the image. To my limited understanding, there is currently no way to detect the difference between a selection stopping at the image boundary and it extending beyond. If my understanding is correct then it would require additional information be saved with the selection masks to accommodate this case (e.g., all selection mask channels expanded to include a one-pixel wide border around the image). 

The ideal would be to have arbitrary-sized selection masks that extend beyond the canvas but this runs into similar difficulties as having the canvas size grow to accommodate user operations (paint strokes, for example). To accomplish it is very difficult and basically requires discarding the concept of an image canvas (i.e., it is a major change). 

I don't think this is a) what a user expects to happen, or b) consistent with doing Select-All and then Shrink by one pixel.

Ethan

A selection always stops at the image boundary. The selection mask is always exactly the size of the image.

The command's current behavior is consistent with "Select->Shrink" if the 'Shrink from image border' option is not checked. If the "Select->Border" command is to be modified, the change should be implemented as a similar option (which is why I proposed identical phrasing).

Oh, so that's what that checkbox was for. Sorry to be ignorant. :)

Ethan

It's not easy to decide what the right thing to do is here.  On one hand, the most common use of selection-border is to get the edge of a selected shape, such as a rectangle, ellipse, or text -- and if the shape runs past the edge of the image, the user probably won't want the image-edge counted as part of the shape.  On the other hand, selecting the edge of the image is probably not too uncommon an action, and users who know about "Select Border" will naturally try to do it the way this bug report describes.  All in all, I vote for NOTABUG on this one, but I can understand why others might feel differently.

(BTW option-checkboxes are not available for menu commands, and having a dialog pop up would be obnoxious.)

Hi, a couple of observations:

1)When you select the whole image and reduce selection by one pixel and select border, it works as expected... Suppose the border width is designated 10, the outer part of the border (which would normally be 5 pixels) is cut off at the edges of the image, and is just 1 pixel as expected, and the inner border is 5

2)If we repeat the above operation, except one side of the selection is on the image edge, than there is no border on that side whatsoever, which doesn't seem to make sense.  If you decide that you want to follow that algorithm (half inner, half outer) for creating a border, which may be a problem itself, than the current behavior seems inconsistent.  If a user does not want a border on one side of the image, he/she can easily remove that part of the selection.  

2)Saul says (above) "I understand your premise but theoretically Select All should create a selection mask that extends out to infinity and a Border operation should not result in a change" 
Note:  if that were true, than why (for all practical purposes) do you get the same result (selection wise) by doing select all or simply manually selecting the whole image

3)I am NOT sure that a user prefers select border to cut off part of the selection every time the border exceeds 1, although this can be useful, it is probably not the MOST useful behavior

4)One solution, which you probably dislike becuase it requires a UI change, is to have a radio button like so:
o Inside selection
o Outside selection

I am trying to be helpful and not critical.  Kudos to the developers, I use GIMP all the time and reccomend it to others.

To to sum up, how about implementing a 'Include image edges' checkbox in the Border Selection dialog, that is to be checked by default?

Since I think not including image borders can be considered a special case (after all, selecting things outside of the image is quite special), I think it would make sense to have it checked by default.

Created attachment 83608 [details] [review]
*incomplete* patch

This is an incomplete patch of my suggestion, the only thing missing is making the border algorithm take the `include_edges' boolean into account.

Any comments on the patch is of course already now welcomed.

I intend to finish this one, but anyone who already has an understanding of the algorithm can definitely add the last things, since understanding 300 lines of mathematical atomic operations can be non-fun ;)

Are you kidding? The "only" thing missing... :-)

Anyway, there is already a parameter like that. The new option
should be consistent with shrink's "edge lock", at least in the code.
This doesn't affect how we name the button's label in the dialog.

Oh, right, didn't think of that.

But now that I do, why can't we have two seperate booleans? I mean, this is two different tools, so why have them share memory? It's not like it would consume huge amount of resources to have it that way. Wouldn't it be more confusing for users if the tools were interconnected for no apparent reason?

Or am I disregarding something?

I meant not to share the booleans, but that the new stuff should perhaps
be called "edge-something" too. Maybe I'm just overly pedantic again :)

Ah ok. Well that's totally reasonable, I will rename it in the final patch.

Do we really need a toggle here? In my opinion there's no point in asking the user to make a decision here. We should just fix the behavior.

I _am_ fixing it.

The user could want either behavior, so a toggle makes perfect sense, since there is no "fit for all" behavior.

I think we should first discuss if we can't find a "fit for all" behavior here. I don't see no point in the current behavior. In my opinion the feature is simply broken and needs to be fixed. There is no need for a change in the user interface. The arguments that have been brought up to support the addition of a toggle are based on the wrong assumption that Select->All would create an infinite selection.  That is not the case. The selection mask stops at the image border.

Look at example screenshot of problematic case, here the user most likely expects the edges to be 'sticky'. On the other hand, Select All + Border should not.

Are you suggesting that we should develop a mega advanced algorithm which, with high probability, predicts what functionality the user wants, and offer no way of correcting possible faulty predictions?

This is a trivial UI change, it is local to a small dialog, and does not interfere with the whole UI experience.

The screenshot shows a selection tool in action. Still the selection that is created when the tool is executed does not extend beyond the image.

I am not going to reject a UI change, even though we are in string freeze and should really also freeze the UI for 2.4. But we should at least check if another option is really needed. It is not only going to make the UI more complex. The complexity of the code is also going to double with each option.

Created attachment 83770 [details]
attachment#1 [details] to comments by Ben

Created attachment 83772 [details]
attachment#2 [details] to comments by Ben

Created attachment 83773 [details]
attachment#3 [details] to comments by Ben

It seems there are too many questions floating around...  After some reflection, I can see a number of distinct issues being discussed.  I would also suggest that no matter how you resolve the individual issues, the select-border function is still inconsistent and should be fixed so that it is consistent.

select-border = SB for brevity

According to definition, SB creates an equal inner and outer border around the selection mask, NOT the portion of the selection that falls within a layer.  (see "attachment#1 [details]-mask-illustration") 

Martin, your comment regarding a "sticky" edge implies a user expects SB to perform differently than it is defined.

Now assuming the definition holds true, see "attachment#2 [details]-inconsistency".  Note how in both cases the selection mask has the same shape, but one side lies on the image boundary, that side does not get a border as expected.  This is a separate issue from any other ones, like the "sticky edge" concept.

Next we should examine the proposed problematic case Saul described, and you will find it is not a problem at all, it should be expected actually.  see "attachment#3 [details]-problem-case"  I have shaded the selection mask green for clarity.  Note what does happen, and what should happen, according to GIMP selection behavior and the SB definition.  Now Martin, if you look at what I think should happen in the attachment, you should realize that there is no "sticky edge" on the image border...  Why? because the selection mask actually stops there, as opposed to the case where there is a layer inside the image and the selection exceeds the layer boundaries, but not the image boundary. This is simply GIMP's normal behavior: selection masks are forcibly constrained to the image borders.

Now to sum up, the SB function should always use the selection mask as a reference point for it's border creation, which clearly it doesn't always do, since for some reason, the a section mask flush with the image border is treated differently and excluded as if (like Saul described) the selection mask extended  infinitely beyond the image edge. Shouldn't this function be consistent first, before you delve into the resulting cases that you find undesirable?  There is no reason I can find for the image border to be strangely different such that a selection mask on that edge does not get a border at all.

Now onto the issues that a consistent SB function might create.
1) Martin's problem: a user expects a border to be created NOT around the selection mask but around the portion of that mask which intersects with the layer or image canvas.  Thus, I presume Martin would prefer, that when the SB is performed, that GIMP first "theoretically" crop the selection mask to the layer boundary and then proceed.  However, this is a NEW behavior, not consistent with  how SB is defined, keeping in mind that a selection mask exceeding image boundaries is NOT current "cropped" by SB, but is simply never allowed to exceed the image boundary in the first place.  SB is not currently expected to alter (or choose the "ideal" portion of) the selection mask before creating a border.

2) Saul's problem: Saul seems to think that a selection mask should be allowed to exceed the image canvas/boundary.  If this were permitted, SB would perform differently, it's true, but it would still follow the shape of the selection mask both before and after this modification.

3) If select-all is performed, the mask is the size of the entire image rather than the size of the current layer. A consistent SB function would create an inner border (GIMP would crop the outer part as expected) along the boundary of the image instead of the layer boundary.  This is a more generalized case of Martin's issue.

4) If a whole image is selected and SB function is called with a border width of 1px, no border will result because a 1px border would normally be placed on the exterior of an image (where GIMP crops it)

As a final summary:
-Shouldn't SB be consistent first, before you address the other issues
-Martin's issue could be addressed by a check box which asks if the selection mask should be cropped to the current layer before creating a border.  I am sure there is a reasonably understandable way to say that and users may like having that option.  Of course, a person could always crop the selection manually before performing SB.  The reverse behavior is much more difficult to accomplish manually.
-The only way to solve Saul's issue, that I can see, is to change the basic behavior of GIMP and allow selection masks to exceed the image boundary.  In practice a user could easily resolve this issue by temporarily increasing the canvas size.
-problem (3) is related to Martin's issue
-I have no suggestion for problem 4

Just my take on this, please forgive me if I misunderstood the statements of others here.

Ben W.

I looked at the original bug raised.

So, select all selects that whole layer. Can select border just automatically grow the layer as needed?

Looks to me that solves the original bug.

Thank you Ben for making a good summary of the problems of this bug.

I think we can ignore the "Should select all select an virtual infinite selection mask" issue, mainly because there is no way for SB or any other tool to tell the difference bewteen 'SA' and e.g. 'Rectangle Select over the whole image', but also because I dont think it makes sense to make such a selection anyway.

The definition? Is there an official definition of the Select -> Border? In any case, I think your definition is very reasonable.



Me and Sven discussed this issue on IRC after his last comment in this thread, and Sven made a good point about that the special functionality one could be interested in, that is, the Before -> [What really happens] chain in attachment #2 [details], can be achieved by using a Shrink/Expand/Difference/Union/etc-combo. I didn't agree then, but I have now changed my mind.

First of all I would like to clarify that I don't think Select All should theoretically select _all_, including things that doesn't exists (i.e. pixels outside of the image). I think object of the idea that Select All should theoretically select everything outside the image.

(In reply to comment #24)
> Now to sum up, the SB function should always use the selection mask as a
> reference point for it's border creation, which clearly it doesn't always do,
> since for some reason, the a section mask flush with the image border is
> treated differently and excluded as if (like Saul described) the selection mask
> extended  infinitely beyond the image edge. Shouldn't this function be
> consistent first, before you delve into the resulting cases that you find
> undesirable?

That is the precise intention of my patch, and always was.

> There is no reason I can find for the image border to be
> strangely different such that a selection mask on that edge does not get a
> border at all.

?? You had an example in attachment #2 [details], [What happends]. I can see that user would like what happens to actually happen.

The plans for my current patch would also add an option that lets the user.

> Now onto the issues that a consistent SB function might create.
> 1) Martin's problem: a user expects a border to be created NOT around the
> selection mask but around the portion of that mask which intersects with the
> layer or image canvas.  Thus, I presume Martin would prefer, that when the SB
> is performed, that GIMP first "theoretically" crop the selection mask to the
> layer boundary and then proceed.

Nope, I don't want that behaviour, I want the selection mask boundaries as they are. What my patch will do is simply to make the user chose functionality between "the definition functionality" and "sticky edges functinoality"

> As a final summary:
> -Shouldn't SB be consistent first, before you address the other issues

Yes, and my patch as intentioned will.

> -Martin's issue could be addressed by a check box which asks if the selection
> mask should be cropped to the current layer before creating a border.

Again, I don't want this functinality, and I definitly don't want to write a patch that implements it ;)

It is important to note that everything SB can do, could be done manually with Shrink, Expand, Add to selection, Substract from selection, Union of selection etc. Therefore I am only doing the least work necessary to close this bug (and make GIMP 2.4 one step closer). However, since the sticky edges functionality is already there, I think it would be reasonable to use the code that as an option, since it probably is not much more work anyway because of the already-is-there-functionality.

But then, the border_region code in gimp/app/paint-funcs/paint-funcs.c is _really_ messy because of the lack of comments and explanations of what the variables means. But since the code probably has lots of bugfixed, it would probably be fatal to rewrite it.

I have sent a mail to Jay Cox and asked him to explain the algo, let's hope he answers. I will upload the latest pach (in which I fixed the code according to suggestions by mitch.

Since I will be busy the coming week, anyone who wants to make an attempt at the border_region code is welcommed.

Created attachment 83806 [details] [review]
*incomplete* patch (improved)

The same things missing as in the first, but this one has cleaner code.

"Me and Sven discussed [...]" to "[...] everything outside the image." was part of a sketch of a reply and was not intended to be included in the reply itself. Therefore, please ignore it when you read my reply, or the reply will appear as confusing. Sorry for the invonvenience.

In response to your comments Martin:

Frankly, I had no clear idea what your patch did, except that it seemed people were talking about another issue and not discussing the first problem (as I saw it)...  I did a little C++ a couple years ago, and never got past console programming, and I lent my huge book to someone and never got it back :(  I looked at your patch and I guess with enough time and effort I might figure it out, but I am still planning on submitting some revisions for the website.

Anyway, I didn't mean to misidentify your intentions...  Besides, the issue that I called "Martin's issue" was not supposed to be a criticism, I was just giving you credit for the idea.

The only thing I don't understand about your response is when you refer to attachment#2 [details] and say that what does happen is expected.  I don't understand that.  Look at the top "Before" picture and notice how I cropped off the right side of the selection.  Then see what happens when a border is selected.  Now look at the 2nd "Before" picture.  I simply moved both and the layer and the selection so that they were flush with the edge of the image.  Then see what does happen.  Why should a user expect that because that very same selection is on the image edge, no border will result.  If the selection had been but 1px away from the image boundary, a normal border would have resulted.

Also, if the select-border function were consistent with the definition I proposed (I thought the gimp manual defined SB this way), the sticky edges functionality that you refer to would only be applicable to 1 specific case, namely, when a select exceeds the layer boundaries for a layer smaller than the image canvas.  It adds nothing to the function of SB in relation to a selection bordering the image canvas on a layer the same size as the canvas.  In the first case, sticky edges asks the selection to be cropped to the layer size.  In the second case, GIMP already made sure the selection was cropped to the layer size before SB is ever called.

Ben W.

(In reply to comment #29) 
> Frankly, I had no clear idea what your patch did, except that it seemed people
> were talking about another issue and not discussing the first problem

No problem, I didn't expect anyone to read and understand the patch, I was only refering to it.

> Anyway, I didn't mean to misidentify your intentions...  Besides, the issue
> that I called "Martin's issue" was not supposed to be a criticism, I was just
> giving you credit for the idea.

Heh, well, it wasn't my idea ;) I didn't think of it until you mentioned it.

> The only thing I don't understand about your response is when you refer to
> attachment#2 [details] [edit] and say that what does happen is expected.  I don't understand
> that.

I must have expressed myself a bit clumsy; I did not mean that [what happends] should be the default, I was only saying that [what happends] *could* be a wanted result. A better example is the 'Example screenshot of a problematic case'-image attached in the thread. Again: The user *could* want the current behavior in such a situation, but that should *not* be the default, but a _special case_. My point is that since the functionality is there already, it is better to keep it as a special case-option that to remove it completely.

> Also, if the select-border function were consistent with the definition I
> proposed (I thought the gimp manual defined SB this way), the sticky edges
> functionality that you refer to would only be applicable to 1 specific case,
> namely, when a select exceeds the layer boundaries for a layer smaller than the
> image canvas.  It adds nothing to the function of SB in relation to a selection

Hmm, no, it appears as if you misunderstood me. To me the layer is irrelevant, all that matters is the image edges. Again, see 'Example screenshot of a problematic case'. But yes, this is more or less 1 specific case, but again, the functionality in code is already there, so why not keep that as a special-case-option?


Also, I am in no way offended by the discussion, it's just that I'm a Swede so my responses might sound upset even though they aren't :)

Re: Comment#30

Martin, I believe I understand precisely what you mean, and I thought it over extensively, but I still believe there is a slight difficulty.  However, I will soon no doubt be considered an annoyance if I continue this discussion any further.  I just don't think your solution covers every possible case, there is still one special case where select-border will behave inconsistently.

Note, if you add an option like you said, I would not suggest that you are making SB inconsistent, rather I believe you intend to allow the user to choose between two different behaviors, each one consistent according to its particular algorithm. So by all means, add the option, keep that functionality as another option, etc.  I just feel, like in the beginning, the patched SB function will still be inconsistent when your new option is unselected.

Imagine for a moment that your option is unselected or unavailable and a user performs SB.  A user may rightly ask why:
1) I have a small layer inside an image and I create a selection which borders the layer on one side.  When I do SB, I get a "half" border along the side of the layer where the selection was on the layer boundary
2) I crop the image to the size of the aforementioned layer and create the exact same selection and then perform SB, this time there is no "half" inner border along the side of the layer where the selection was on the layer boundary.

Your proposed option mixes apples and oranges, so to speak.  SB does something inconsistent with it's usual behavior normally, but when your option is selected, now SB is consistent with its usual definition is one special case, and then does something different for all other cases.

Anyway, if I still don't make sense now, I will just drop it and not interfere with your plans any further.  Maybe your patch actually solves this issue after all.

One final comment:  The only reason there is any problem at all, is because selection mask are not permitted to be larger than the image canvas.  (forget the infinity concept).  I doubt anyone cares to make such a major change, but the only reason for the strange behavior and inconsistency is because a selection is forcibly cropped at the image edge.  If it weren't SB would be so simple and understandable.  You would simply ask the user if he/she wanted a border to created for the selection mask, or for the intersection of the selection mask and the layer (no matter what the layer size).  Of course it would have said in an understandable way.  But as it is, your option will, I suppose cover all the possibilities, but won't leave SB really consistent about either of it's two user-"chooseable" behaviors.  

Do I make any sense?  If not, just say no, and don't waste time trying to set me straight :)  I do wonder though, does photoshop/psp allow selection masks to extend beyond the image boundary.  After all, a layer can extend outside the image canvas. Note, that part beyond the canvas does exist.  If you expand the canvas, you will see the rest of the layer.  Not so with a selection, make one larger than the canvas and expand the canvas, you will find the missing part of the selection is still cut off.  I understand that would be a very major change, and it would probably cause other problems too that I haven't thought of for other parts of GIMP.  Oh well, a little thought about this change, and I can see the enormous can of worms it would create.

It would be nice it at least somebody said, I know what you mean, but I still think this other way will be ok, even if it really isn't consistent/predictable for all cases.

This is my last attempt to explain.  It's not THAT big a deal anyway...

Ben W.

Actually, I think I understand what you mean now :D

You are saying that the inconsistency of the current behavior will still be present after my patch, when the user selects the special case-mode (sticky edges). That is correct. But that will be subject to a separate bug report, and will as you say be much work for little gain.

I had a chat with Martin on the IRC.

We agreed that simply fixing the bug is a good thing. If a selection edge falls on the layer edge (sticky as Martin names it) select border will simply work for that border too.

We also agreed to not put a new checkbox in of select border dialog, in order to keep is sweet, simple and consistent for the user.

Let's simply fix this bug.

Created attachment 83938 [details] [review]
Complete-Patch-Select-Border-Algorithm.patch

Hopefully this is without flaws, but I might have missed some use case. Let me know and I'll fix it.

I probably should elaborate on what it does.

The patch makes no GUI changes. The only thing patched is the SB algorithm itself. While I were at it I cleaned up comments and added a bunch of them to save time for future programmers who might make changes/do ports.

I am sorry, but this is *exactly* the same thing as the toggle in the
"shrink selection" dialog. There are totally useful use cases for both
behaviors. I really don't see what the problem with the toggle is.

Simply changing it to do the other thing is not a fix, it's a real
change in behavior without any way to go back to the old one.

Why should the toggle in the border dialog be less needed/useful
than in the shrink dialog. At least I have found myself using the
shrink toggle several times since it was introduced, and it would have
been a major pita to acheive the same effect without it.

Created attachment 83960 [details] [review]
New complete patch, with checkbox added

There is no technical problem at all, I just got a requests from a translator to not create a new translation string, and since we're in string freeze I think it's a reasonable request.

For me personally it's no problem at all, I think there should an option the the Border Select dialog.

Here's the (even more :p) complete patch, including the new checkbox in the Border Selection dialog.

Well, I am not a translator, but the old, broken, algorithm is too undefined to offer as a feature.

For instance: is this about the image edge or the layer? I can reproduce no-border-selection
with a select-all on a smaller-than-image layer.

When that is defined, we get a step closer to including the checkbox.

I don't see any broken behavior, where is the bug in your opinion?

Created attachment 83969 [details] [review]
fixed version of previous patch

Here's the PDB- and ChangeLog fixed version.

Fixed in SVN:

2007-03-05  Michael Natterer  <mitch@gimp.org>

	Makes default Select -> Border behaviour consistent, and makes
	'sticky image edges' optional by adding a checkbox in the Border
	Selection dialog. Patch by Martin Nordholts. Fixes bug #350009.

	* app/actions/select-commands.c (select_border_cmd_callback)
	(select_border_callback): Added edge-lock checkbox to dialog and
	modified calls accordingly.

	* app/paint-funcs/paint-funcs.c (border_region)
	(compute_transition): Fixed algorithm. (compute_transition is a
	helper function to the algorithm). Also clarified many parts of
	the algorithm with comments.

	* app/paint-funcs/paint-funcs.h
	* app/core/gimpchannel.[ch]
	* app/core/gimpselection.c: Added gboolean edge_lock to function
	calls/signatures.

	* app/pdb/selection_cmds.c: Regenerated. 

as discussed with Martin and Mitch, the text on the new checkbox is not good enough for general release.

first a bit of background to take into consideration:

1) make a selection oval, then move it so that a bit of it goesover the left and top image border, but not over the top-left corner. although technically there are now selection pixels outside the image area, for the user it looks like (and therefore it is) that two parts of this selection run exactly along the image border.

2) the new checkbox only makes sense if there are any selection parts run exactly along the image border.

now, making the checkbox sensitive based on condition 2) was not doable. OK.
in that case we have to display a precondition/cetae a context for the checkbox, by putting a label above it:

"Where selection runs along image border:"

and label the checkbox based on 1):

"Do not border-select these parts"