This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 138146

Summary: Need version of SHOW_REUSE which doesn't grab the focus
Product: platform Reporter: ivan <ivan>
Component: TextAssignee: mslama <mslama>
Status: RESOLVED FIXED    
Severity: blocker CC: anebuzelsky, apireviews
Priority: P2 Keywords: API_REVIEW_FAST
Version: 6.x   
Hardware: All   
OS: All   
Issue Type: DEFECT Exception Reporter:
Attachments: Patch for this issue
Update deprecated tags for old Line constants
Use non abstract Line.show(ShowOpenType, ShowVisibilityType, int)
Give more specific warning message

Description ivan 2008-06-24 20:20:02 UTC
The difference between Line.SHOW_SHOW and SHOW_GOTO is that SHOW_SHOW doesn't
cause the edito to grab focus whcih SHOW_GOTO does.

There is no such variation for SHOW_REUSE and SHOW_REUSE_NEW.

See #132671 and #108834 for discussions of how sunstudio needs variations on SHOW)
that don't grab focus.

BTW, itsn't it time to regularize the flags to show ... the space it covers
is rather multidimensional and adding individual enumerations is not a scalable
solution nor can it anticipate all potential uses.

Example of what I mean ...
Introduce bit fields 
   SHOW_BIT_OPEN
   SHOW_BIT_FOCUS
   SHOW_BIT_FRONT
   SHOW_BIT_REUSE
Redefine existing constants:
   SHOW_SHOW = SHOW_BIT_OPEN
   SHOW_GOTO = SHOW_BIT_OPEN | SHOW_BIT_FOCUS
   SHOW_TOFRONT = SHOW_BIT_OPEN | SHOW_BIT_FOCUS | SHOW_BIT_FRONT
   SHOW_TRY_SHOW = <none of the flags set>
   SHOW_REUSE = SHOW_BIT_REUSE |  SHOW_BIT_FOCUS
   SHOW_REUSE_NEW = SHOW_BIT_OPEN | SHOW_BIT_REUSE | SHOW_BIT_FOCUS

Even though this way the numbers assigned to the variables will be different
it should not cause a binary incompatibility.

This way instead of creating yet another enum I could use
   SHOW_BIT_REUSE
Comment 1 mslama 2008-06-25 14:28:43 UTC
This suggestion seems reasonable to me. Current constants in Line class would stay there for backward compatibility and
new bit constants would be added. Main reason is to split behavior into categories as described above so that we need
not provide constanst for all possible combinations. I need opinion from another people before I start to implement this
change.
Comment 2 Jesse Glick 2008-07-02 00:46:52 UTC
[JG01] Use an enumeration rather than integer flags. You can use EnumSet to compose them.
Comment 3 mslama 2008-07-04 15:00:50 UTC
Yes EnumSet is fine. I checked SHOW_REUSE and SHOW_REUSE_NEW and these have different behavior so they must have their
own new flags. It means it is not possible to use suggested combinations. In addition 
Comment 4 mslama 2008-07-04 16:01:16 UTC
    /** Replacement for constants SHOW_TRY_SHOW, SHOW_SHOW,
     * SHOW_GOTO, SHOW_TOFRONT, SHOW_REUSE, SHOW_REUSE_NEW. It is to provide full control
     * over show method behavior without need to add new constant for missing flag combination.
     * SHOW_TYPE_OPEN if set new editor is opened when it is not yet opened
     * SHOW_TYPE_FOCUS if set editor is focused/activated it implies SHOW_TYPE_FRONT so using SHOW_TYPE_FOCUS and
     *                 SHOW_TYPE_FRONT is redundant
     * SHOW_TYPE_FRONT if set editor is fronted/selected
     * SHOW_TYPE_REUSE if set replaces editor marked for reuse (ie. editor opened with last SHOW_TYPE_REUSE
     *                 or SHOW_TYPE_REUSE_NEW) and if editor is not yet opened marks it for reuse
     * SHOW_TYPE_REUSE_NEW if set ignores editor marked for reuse and resets reference to editor marked for reuse
     *                 and if editor is not yet opened marks it for reuse
     *
     * Note: SHOW_TYPE_REUSE and SHOW_TYPE_REUSE_NEW flags cannot be used together.
     *
     * Note: Any modification of editor marked for reuse resets reuse flag. There is one global static reference
     * so only one or none editor can be marked for reuse.
     *
     * Replacement for old constants:
     * SHOW_TRY_SHOW {}
     * SHOW_SHOW {SHOW_TYPE_OPEN}
     * SHOW_GOTO {SHOW_TYPE_OPEN, SHOW_TYPE_FOCUS}
     * SHOW_TOFRONT {SHOW_TYPE_OPEN, SHOW_TYPE_FRONT}
     * SHOW_REUSE {SHOW_TYPE_REUSE, SHOW_TYPE_FOCUS}
     * SHOW_REUSE_NEW {SHOW_REUSE_NEW, SHOW_TYPE_FOCUS}
     */
    public enum ShowTypes {
        SHOW_TYPE_OPEN,
        SHOW_TYPE_FOCUS,
        SHOW_TYPE_FRONT,
        SHOW_TYPE_REUSE,
        SHOW_TYPE_REUSE_NEW
    };

Flags SHOW_TYPE_REUSE and SHOW_TYPE_REUSE_NEW imply SHOW_TYPE_OPEN but as it makes no sense to use these 2 without
SHOW_TYPE_OPEN I do not split 'open' and 'reuse'. It means it makes no sense to combine {SHOW_TYPE_OPEN,
SHOW_TYPE_REUSE} or {SHOW_TYPE_OPEN, SHOW_TYPE_REUSE_NEW}. Also flags {SHOW_TYPE_REUSE, SHOW_TYPE_REUSE_NEW} cannot be
combined.

Possible combinations are
{}
{SHOW_TYPE_OPEN}
{SHOW_TYPE_OPEN, SHOW_TYPE_FOCUS}
{SHOW_TYPE_OPEN, SHOW_TYPE_FRONT}
{SHOW_TYPE_REUSE}
{SHOW_TYPE_REUSE, SHOW_TYPE_FOCUS}
{SHOW_TYPE_REUSE, SHOW_TYPE_FRONT}
{SHOW_TYPE_REUSE_NEW}
{SHOW_TYPE_REUSE_NEW, SHOW_TYPE_FOCUS}
{SHOW_TYPE_REUSE_NEW, SHOW_TYPE_FRONT}

So this is not pure bit mask with all possible values. So there is question if this is right way. In any way current
impl does not allow to separate 'open' from 'reuse'. Would it make any real sense to separate it?
Comment 5 mslama 2008-07-04 16:04:32 UTC
I missed following combinations:
{SHOW_TYPE_FOCUS}
{SHOW_TYPE_FRONT}
Comment 6 mslama 2008-07-04 19:05:28 UTC
As I look at it now open and visibility types are well separated. So I could use 2 params instead of one.
    public enum ShowOpenTypes {
        SHOW_OPEN_TYPE_NONE,
        SHOW_OPEN_TYPE_OPEN,
        SHOW_OPEN_TYPE_REUSE,
        SHOW_OPEN_TYPE_REUSE_NEW
    };
    public enum ShowVisibilityTypes {
        SHOW_VISIBILITY_TYPE_NONE,
        SHOW_VISIBILITY_TYPE_FRONT,
        SHOW_VISIBILITY_TYPE_FOCUS
    };
These 2 values can be combined in any way and cover all old cases.
Comment 7 ivan 2008-07-04 20:42:19 UTC
[IS01] Just to make sure ...

    public enum ShowVisibilityTypes {
        SHOW_VISIBILITY_TYPE_NONE,
        SHOW_VISIBILITY_TYPE_FRONT,
        SHOW_VISIBILITY_TYPE_FOCUS              // implies SHOW_VISIBILITY_TYPE_FRONT
                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    };

[IS02] Can all the combinations be implemented?
       If not, What happens to invalid combinations?
Comment 8 mslama 2008-07-04 21:17:52 UTC
is01: Yes
is02: Yes all couples are possible. First 'open' part is done (ie. one action from ShowOpenTypes). And then visibility
action is performed. These 2 actions are well separated.
Comment 9 mslama 2008-07-07 15:53:24 UTC
Created attachment 64001 [details]
Patch for this issue
Comment 10 mslama 2008-07-07 16:00:50 UTC
Jarda agreed to go for fasttrack.
Comment 11 mslama 2008-07-07 16:06:07 UTC
Please someone read new javadoc if it is clear enough. Especially REUSE and REUSE_NEW have more complex behavior. I
added note about one global reference to reusable editor to make things clearer. IMO if some developer is new to this
API (like me) it is not obvious.
Comment 12 Jesse Glick 2008-07-08 23:54:41 UTC
[JG02] Can you use column == -1 (no particular column) for the new method? The Javadoc does not say.


[JG03] @deprecated messages should specify which combination to use. Please also consider updating existing modules to
use the preferred API style.
Comment 13 mslama 2008-07-11 09:05:15 UTC
Ad JG02: Does it make any sense to use -1 for column? What should it do? It means undefined (random or first column)
column will be selected. As editor cursor must be somewhere. It is as show without column translated to 0 column.

Ad JG03: Ok I will add new combination to @deprecated messages. There is translation table in javadoc for ShowOpenType.
If I add this to old constants shall I remove it from this place? I will try find usages on full IDE to find out
Line.show usages and see. I will probably file issues and rewrite usage if module owner(s) will agree. I will get to it
next week.

I plan to commit this during weekend.
Comment 14 Jesse Glick 2008-07-11 15:46:39 UTC
To JG02 - that's fine but it is not specified in the Javadoc.
Comment 15 mslama 2008-07-11 18:18:28 UTC
javadoc of show without column parameter says: "Shows the line (at the first column)" It is enough IMO. I moved
replacement constants to @deprecated tag for every old constant. I will commit this tomorrow evening.
Comment 16 mslama 2008-07-11 18:19:22 UTC
Created attachment 64352 [details]
Update deprecated tags for old Line constants
Comment 17 mslama 2008-07-11 18:25:42 UTC
Correction for one link here:
    /** Focuses or opens given editor, marking it as reusable editor if it
     * was not opened before. Similar to {@link #SHOW_REUSE} but ignores
     * currently reusable editor.
     * @see #show(int) <code>show</code>
     * @since org.openide.text 6.14
     * @deprecated Deprecated since 6.21. Use {@link ShowOpenType#SHOW_OPEN_TYPE_REUSE_NEW}
     * and {@link ShowVisibilityType#SHOW_VISIBILITY_TYPE_FOCUS} instead.
     */
    public final static int SHOW_REUSE_NEW = 5;
Comment 18 Jesse Glick 2008-07-11 18:41:12 UTC
To JG02 - no, it's not enough, because "-1" is not mentioned anywhere, yet that is what you in fact need to pass to the
new method if you wish to not change the column. That, or add a new method override that takes your new constants but
not a column number.
Comment 19 mslama 2008-07-11 18:56:48 UTC
Ahh I see now: if (column >= 0) { in code. It is not mentioned anywhere. I will add: Pass -1 if you do not want to
change selected column. It applies to old show method too so I add the same to it.
Comment 20 mslama 2008-07-12 18:55:36 UTC
Fixed as 9d3449089017.

-1 for column added to javadoc of method show.
Module version increased in module manifest - it was missing i diff.
Comment 21 mslama 2008-07-14 12:48:59 UTC
Sigtest found incompatible change: I did not know it is not possible to add abstract method into API. So I had to
provide simple impl of Line.show(ShowOpenType openType, ShowVisibilityType visibilityType, int column). It fallbacks to
show(int,int) and logs warning when not supported params are passed into method. Proper impl is provided by
org.openide.text.EditorSupportLineSet.SupportLine. Diff attached. Sigtest now passes. I put it to main as it breaks
continuous build.
Comment 22 mslama 2008-07-14 12:50:26 UTC
Created attachment 64424 [details]
Use non abstract Line.show(ShowOpenType, ShowVisibilityType, int)
Comment 23 mslama 2008-07-14 12:55:19 UTC
Created attachment 64425 [details]
Give more specific warning message
Comment 24 mslama 2008-07-14 13:00:46 UTC
Fixed in main 5f55df5841aa
Comment 25 Quality Engineering 2008-07-17 04:32:53 UTC
Integrated into 'main-golden', available in NB_Trunk_Production #324 build
Changeset: http://hg.netbeans.org/main/rev/5f55df5841aa
User: Marek Slama <mslama@netbeans.org>
Log: #138146: Do not add abstract method to API.
Comment 26 Jesse Glick 2008-07-21 17:39:49 UTC
I suppose it is too late to comment that

[JG04] SHOW_VISIBILITY_TYPE_ and SHOW_OPEN_TYPE_ prefixes on enum constant names are redundant and annoying. Calling

.show(Line.ShowOpenType.SHOW_OPEN_TYPE_OPEN, Line.ShowVisibilityType.SHOW_VISIBILITY_TYPE_FOCUS)

is just verbose and no clearer than

.show(Line.ShowOpenType.OPEN, Line.ShowVisibilityType.FOCUS)
Comment 27 mslama 2008-07-21 21:12:56 UTC
It is after feature freeze. Tonda? Cosmetic change?
Comment 28 mslama 2008-07-21 21:16:44 UTC
As I did not get into rewriting client code yet this rename would limited to openide/text only.
Comment 29 Jesse Glick 2008-07-21 21:50:19 UTC
...and now o.apache.tools.ant.module also.
Comment 30 Antonin Nebuzelsky 2008-07-22 09:40:19 UTC
> Tonda? Cosmetic change?

Better do it now before it gets widely used. So, do it.
Comment 31 mslama 2008-07-22 14:36:01 UTC
Fixed as 8c2b7fcc16bd and f2f8bd75346d in core-main.
Comment 32 ivan 2008-09-05 02:03:00 UTC
I'm sorry, but after all this good effort the original problem
I had is still not solved.

When I use 
     line.show(Line.ShowOpenType.REUSE, Line.ShowVisibilityType.FRONT);
focus will get grabbed away in the case where a tab is actually reused.

Alter the testcase project provided in 132671 with this:

    private void showInEditor(Line line, boolean reuse) {
	if (reuse)
	    // OLD line.show(Line.SHOW_REUSE);
	    line.show(Line.ShowOpenType.REUSE, Line.ShowVisibilityType.FRONT);
	else
	    // OLD line.show(Line.SHOW_SHOW);
	    line.show(Line.ShowOpenType.OPEN, Line.ShowVisibilityType.FRONT);
    }

then issue commands 
    edit -r filea
    edit -r fileb
    edit -r filec    will reuse and grab focus away
Comment 33 mslama 2008-09-10 11:57:23 UTC
Issue #132671 contains testing module.
Comment 34 mslama 2008-09-10 12:16:02 UTC
I tried and cannot reproduce your problem. I modified source in test module as described:
    private void showInEditor(Line line, boolean reuse) {
        if (reuse) {
            //line.show(Line.SHOW_REUSE);
            line.show(Line.ShowOpenType.REUSE, Line.ShowVisibilityType.FRONT);
        } else {
            //line.show(Line.SHOW_SHOW);
            line.show(Line.ShowOpenType.OPEN, Line.ShowVisibilityType.FRONT);
        }
    }
then use build and run.
Created 3 source files a,b,c and used commands as mentioned above
cd (to get to home dir)
pwd (to check current work dir)
edit -r a
edit -r b
edit -r c
edit -r a

Focus stays in shell window not moved to editor. Anyway please file separate issue to avoid long issue and give steps to
reproduce it there. I am on Linux, latest trunk dev build, JDK 6u10:
  Product Version         = NetBeans IDE Dev (Build 080910)
  Operating System        = Linux version 2.6.24-21-generic running on i386
  Java; VM; Vendor        = 1.6.0_10; Java HotSpot(TM) Client VM 11.0-b15; Sun Microsystems Inc.
  Runtime                 = Java(TM) SE Runtime Environment 1.6.0_10-b30

I have no idea what is the difference causing this. I tried even clean and build.