Hi Sean, Pavel,
For ease of review, I've uploaded a webrev [1] with a slightly updated
form of the current suggested fix and testcase, in which:
* The checking code is moved into updateSystemSelection, as
suggested
* The testcase is in a (hopefully) sensible location
* The testcase copyright text is as previously agreed
* The copyright year is corrected (to 2011)
I'll look to track this conversation for any further changes to the
suggested fix, and update the webrev as necessary.
Regards, Neil
[1] http://cr.openjdk.java.net/~ngmr/7049024/webrev.01/
On Mon, 2011-09-26 at 14:45 +0400, Pavel Porvatov wrote:
> Hi Sean,
> > Hi Pavel,
> >
> >
> > Thanks for new comments. I was looking at test\javax\swing
> > \JTextArea\6940863
> > and copied the copyright from there, and just put the initialization
> > part of the swing
> > code into EDT. I didn't find realSynch in Toolkit class so used
> > synch. I'll modify
> > that.
> >
> The realSync is a SunToolkit method.
> > About the first comment, do you mean to put the testcase and the
> > fix together
> > in a webrev ?
> >
> Yep! It's hard to reconstruct fix from different mails. Don't forget
> to select an appropriate folder for the test...
>
> Thanks, Pavel
> >
> > 2011/9/26 Pavel Porvatov <pavel.porvatov at oracle.com>
> > Hi Sean,
> > > Hi Pavel,
> > >
> > >
> > > Thanks for the comments. I modified the testcase
> > > according to the comments,
> > > please have a look at it again.
> > > I tried to run with jtreg and it runs well.
> > >
> > Looks much better. Still have several comments:
> >
> > 1. Could you please sent complete fix as a webrev, but not
> > parts of the fix as a single file?
> >
> > 2. Date of copyright...
> >
> > 3. You are still using Swing components on non-EDT thread.
> > As I wrote before take a look at the test\javax\swing
> > \JSlider\6848475\bug6848475.java test...
> >
> > 4. Use toolkit.realSync() instead of toolkit.sync(). BTW: as
> > described in javadoc realSync cannot be invoked on the EDT
> > thread...
> >
> > Regards, Pavel
> >
> > P.S. Sorry for my stubborn =) But on some machines not
> > accurate tests actually fail (e.g. on Solaris). Therefore
> > later we must fix tests and that's a really boring task...
> >
> >
> > >
> > > 2011/9/20 Pavel Porvatov <pavel.porvatov at oracle.com>
> > > Hi Sean,
> > >
> > > The have several comments :
> > >
> > > 1. Could you please read
> > > http://openjdk.java.net/jtreg/
> > > is it possible run your test via jtreg?
> > >
> > > 2. There is no copyright in the begin of test
> > >
> > > 3. There are no jtreg tags
> > >
> > > 4. All Swing code must be initialized on the EDT
> > > thread
> > >
> > > 5. Keep test minimal as possible, please. It helps
> > > other people to understand your code.... E.g.
> > > there is no need to create JButton with listener.
> > >
> > > 6. Note that the "frame.setVisible(true)" doesn't
> > > guarantee that after that line Frame is visible,
> > > you should use toolkit.realSync() here
> > >
> > > 7. No TODO, please
> > >
> > > 8. Are you sure your test should pass if
> > > exceptions occurs (see your catch blocks)
> > >
> > > Please take a look at other tests and use them as
> > > a good examples....
> > >
> > > Regards, Pavel
> > >
> > >
> > > > Hi Pavel,
> > > >
> > > >
> > > > I wrote a test case for the behavior of
> > > > DefaultCaret. Please take a look, it is
> > > > attached.
> > > >
> > > >
> > > >
> > > >
> > > > 2011/9/15 Pavel Porvatov
> > > > <pavel.porvatov at oracle.com>
> > > > Hi Sean,
> > > > > Hi Pavel,
> > > > >
> > > > >
> > > > > I'm comfortable with moving the
> > > > > checking
> > > > > into DefaultCaret#updateSystemSelection method.
> > > > > About regression test, I'm not
> > > > > sure how to write, because it contains
> > > > > user operation. Can you
> > > > >
> > > > > give me a similar test so I can write
> > > > > one for this bug?
> > > > >
> > > > Yes, you can find a lot examples in the
> > > > test/javax/swing directory by word
> > > > Robot, e.g. test\javax\swing\JSlider
> > > > \6848475\bug6848475.java. One hint: use
> > > > reflection ONLY if there are no another
> > > > ways to write a test...
> > > >
> > > > Regards, Pavel
> > > >
> > > > >
> > > > >
> > > > >
> > > > > 2011/9/13 Pavel Porvatov
> > > > > <pavel.porvatov at oracle.com>
> > > > > Hi Sean,
> > > > > > Hi Pavel ,
> > > > > >
> > > > > >
> > > > > > I'm sorry I didn't make
> > > > > > update for this bug for a
> > > > > > long time, and here is some
> > > > > > recent investigation. The
> > > > > > scenario is as follows:
> > > > > >
> > > > > >
> > > > > > Suppose we are dragging
> > > > > > "abcde" over TextField tf,
> > > > > > which have "hello dragging"
> > > > > > as
> > > > > > its content. When we are
> > > > > > dragging from start to end,
> > > > > > there is a cursor moving
> > > > > > from
> > > > > > "h" to "g", which means the
> > > > > > place to insert "abcde" if
> > > > > > we drop it.
> > > > > > When we dragging "abcde"
> > > > > > exit tf, there will be a
> > > > > > dragExit event, the tf needs
> > > > > > to
> > > > > > restore its original status
> > > > > > after we drag out. Eg. if
> > > > > > its cursor is between "h"
> > > > > > and
> > > > > > "e" in "hello", which
> > > > > > appears like "h|ello", when
> > > > > > we are dragging over it, it
> > > > > > may like
> > > > > > "hello dr|agging", and when
> > > > > > drag exit, it needs to be
> > > > > > "h|ello" again.
> > > > > > So in dragExit handler, it
> > > > > > calls
> > > > > > javax.swing.TransferHandler.cleanup(false), which
> > > > > > means only to restore the
> > > > > > original state. cleanup
> > > > > > calls
> > > > > > javax.swing.text.JTextComponent.setDropLocation to set the cursor to original
> > > > > > position.
> > > > > > And setDropLocation calls
> > > > > > DefaultCaret.setDot
> > > > > > and DefaultCaret.moveDot
> > > > > > to set the state.
> > > > > > The problem is moveDot
> > > > > > doesn't know this is just to
> > > > > > restore the original state,
> > > > > > it treats the invocation as
> > > > > > an action to select
> > > > > > something. And it
> > > > > > calls updateSystemSelection
> > > > > > which will
> > > > > > call java.awt.datatransfer.Clipboard.setContent. And the selected content
> > > > > > is changed from "abcde" to
> > > > > > the original selected part
> > > > > > of "hello dragging", then
> > > > > > the drop operation finds it
> > > > > > is not the string dragged
> > > > > > and nothing is dropped.
> > > > > >
> > > > > >
> > > > > > So I made a simple
> > > > > > patch(attached) . It just
> > > > > > check if the textField owns
> > > > > > focus
> > > > > > before updateSystemSelection, if it is not focused, it does not treat the moveDot as
> > > > > > a selection action and does
> > > > > > not
> > > > > > call Clipboard.setContent.
> > > > > > This works on Linux,
> > > > > > however, DefaultCaret is
> > > > > > shared by Linux and Windows
> > > > > > while windows doesn't have
> > > > > > this problem. So I don't
> > > > > > think this is a correct
> > > > > > patch, but it brings my
> > > > > > question.
> > > > > >
> > > > > >
> > > > > > I think it is strange for
> > > > > > DefaultCaret to use setDot
> > > > > > and moveDot to restore
> > > > > > original state, especially
> > > > > > moveDot will cause
> > > > > > an updateSystemSelection
> > > > > > operation,
> > > > > > which makes moveDot much
> > > > > > like an action from user
> > > > > > instead of just restoring
> > > > > > state.
> > > > > >
> > > > > >
> > > > > > I'm not sure why it works
> > > > > > well on windows, but I don't
> > > > > > think it is right to call
> > > > > > updateSystemSelection or it
> > > > > > is not right to use setDot
> > > > > > and moveDot to restore
> > > > > > the original state. Is there
> > > > > > any reason for that ?
> > > > > Thanks for the patch! I
> > > > > believe you are right and we
> > > > > shouldn't update system
> > > > > selection clipboard when the
> > > > > component doesn't have focus.
> > > > > I'd like to modify your fix
> > > > > and move checking into the
> > > > > DefaultCaret#updateSystemSelection method:
> > > > > if (this.dot != this.mark &&
> > > > > component != null &&
> > > > > component.hasFocus()) {
> > > > >
> > > > > We also must write regression
> > > > > tests for fixes if possible,
> > > > > so an automatic test is needed
> > > > > as well. Could you please
> > > > > write a test for the fix?
> > > > >
> > > > >
> > > > > > I'm not sure why it works
> > > > > well on windows,
> > > > >
> > > > > That's because Windows doesn't
> > > > > have system selection
> > > > > clipboard...
> > > > >
> > > > >
> > > > > > Is there any reason for
> > > > > that ?
> > > > >
> > > > > No, that's a just bug...
> > > > >
> > > > > Regards, Pavel
> > > > >
> > > > > >
> > > > > > 2011/6/6 Pavel Porvatov
> > > > > > <pavel.porvatov at oracle.com>
> > > > > > Hi Sean,
> > > > > > > Hi,
> > > > > > >
> > > > > > > I reported, but
> > > > > > > the system doesn't
> > > > > > > reply me a bug
> > > > > > > number. It says
> > > > > > > "will give me
> > > > > > > email",
> > > > > > > but I haven't got
> > > > > > > one yet. Is this
> > > > > > > the right process,
> > > > > > > or I might make a
> > > > > > > problem when
> > > > > > > reporting?
> > > > > > >
> > > > > > I don't know why the
> > > > > > system didn't report
> > > > > > bug ID, but your bug
> > > > > > was filed
> > > > > > successfully. You
> > > > > > can find it here:
> > > > > > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7049024
> > > > > >
> > > > > > Regards, Pavel
> > > > > >
> > > > > > >
> > > > > > > 2011/5/27 Pavel
> > > > > > > Porvatov
> > > > > > > <pavel.porvatov at oracle.com>
> > > > > > > Hi Sean,
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > >
> > > > > > > > I
> > > > > > > > have a
> > > > > > > > testcase
> > > > > > > > related
> > > > > > > > to DnD
> > > > > > > > failure
> > > > > > > > with
> > > > > > > > JTextArea and JTextField on linux. The
> > > > > > > > testcase
> > > > > > > > is as
> > > > > > > > follows:
> > > > > > > >
> > > > > > > >
> > > > > > > > /*
> > > > > > > > * DnDTest.java
> > > > > > > > */
> > > > > > > > import
> > > > > > > > java.awt.Color;
> > > > > > > > import
> > > > > > > > java.awt.Component;
> > > > > > > > import
> > > > > > > > java.awt.Dimension;
> > > > > > > > import
> > > > > > > > java.awt.FlowLayout;
> > > > > > > > import
> > > > > > > > java.awt.Frame;
> > > > > > > > import
> > > > > > > > java.awt.event.WindowAdapter;
> > > > > > > > import
> > > > > > > > java.awt.event.WindowEvent;
> > > > > > > >
> > > > > > > >
> > > > > > > > import
> > > > > > > > javax.swing.JTextArea;
> > > > > > > > import
> > > > > > > > javax.swing.JTextField;
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > public
> > > > > > > > class
> > > > > > > > DnDTest
> > > > > > > > extends
> > > > > > > > Frame {
> > > > > > > > Component c;
> > > > > > > > public
> > > > > > > > DnDTest() {
> > > > > > > > super("Single Frame --- AWT Frame");
> > > > > > > > super.setBackground(Color.gray);
> > > > > > > >
> > > > > > > > // set
> > > > > > > > layout
> > > > > > > > here.
> > > > > > > > setLayout(new FlowLayout());
> > > > > > > >
> > > > > > > > c = new
> > > > > > > > JTextArea("JTextArea component");
> > > > > > > > c.setPreferredSize(new Dimension(400, 100));
> > > > > > > > add(c);
> > > > > > > >
> > > > > > > > c = new
> > > > > > > > JTextField("JTextField component(No IM)");
> > > > > > > > c.setPreferredSize(new Dimension(400, 20));
> > > > > > > > add(c);
> > > > > > > >
> > > > > > > > addWindowListener(new WindowAdapter() {
> > > > > > > > public
> > > > > > > > void
> > > > > > > > windowClosing(WindowEvent event) {
> > > > > > > > System.exit(0);
> > > > > > > > }
> > > > > > > > });
> > > > > > > > setSize(850, 360);
> > > > > > > > setVisible(true);
> > > > > > > > }
> > > > > > > >
> > > > > > > > public
> > > > > > > > static
> > > > > > > > void
> > > > > > > > main(String[] args) {
> > > > > > > > new
> > > > > > > > DnDTest();
> > > > > > > > }
> > > > > > > > }
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Reproduce steps:
> > > > > > > > 1. Run
> > > > > > > > the
> > > > > > > > testcase
> > > > > > > > with
> > > > > > > > b143
> > > > > > > > 2. Open
> > > > > > > > a new
> > > > > > > > file
> > > > > > > > with
> > > > > > > > gedit
> > > > > > > > and
> > > > > > > > input
> > > > > > > > some
> > > > > > > > words
> > > > > > > > like
> > > > > > > > "abcde"
> > > > > > > > 3. Drag
> > > > > > > > "abcde"
> > > > > > > > into
> > > > > > > > JTextField and drop it there.
> > > > > > > > 4. Once
> > > > > > > > more,
> > > > > > > > drag
> > > > > > > > "abcde"
> > > > > > > > into
> > > > > > > > JTextField and then move out of the Frame (keep draging) and drag into JTextField again and drop it.
> > > > > > > >
> > > > > > > >
> > > > > > > > Expectation:
> > > > > > > > The
> > > > > > > > second
> > > > > > > > DnD
> > > > > > > > inputs
> > > > > > > > another
> > > > > > > > "abcde"
> > > > > > > > into JTextField.
> > > > > > > >
> > > > > > > >
> > > > > > > > Result:
> > > > > > > > The
> > > > > > > > second
> > > > > > > > DnD
> > > > > > > > inputs
> > > > > > > > nothing
> > > > > > > > into JTextField.
> > > > > > > Yes, looks
> > > > > > > like a
> > > > > > > bug. The
> > > > > > > test case
> > > > > > > works on
> > > > > > > Windows as
> > > > > > > expected.
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Investigation:
> > > > > > > > The
> > > > > > > > JTextArea as well has this problem, and in step 4, if we drag "abcde" over JTextField and then drop into JTextArea, nothing
> > > > > > > > is input
> > > > > > > > into
> > > > > > > > JTextArea either. However, if "abcde" is drag into JTextField or JTextArea directly or when JTextArea/Field are
> > > > > > > > empty as
> > > > > > > > in step
> > > > > > > > 2, it
> > > > > > > > works.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Are
> > > > > > > > there
> > > > > > > > any
> > > > > > > > comments? And can anyone file a bug for it please ?
> > > > > > > >
> > > > > > > Anybody
> > > > > > > can file a
> > > > > > > bug,
> > > > > > > http://bugreport.sun.com/bugreport/
> > > > > > >
> > > > > > > Regards,
> > > > > > > Pavel
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Best Regards,
> > > > > > > Sean Chou
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Best Regards,
> > > > > > Sean Chou
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best Regards,
> > > > > Sean Chou
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Best Regards,
> > > > Sean Chou
> > > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > --
> > > Best Regards,
> > > Sean Chou
> > >
> > >
> >
> >
> >
> >
> >
> >
> > --
> > Best Regards,
> > Sean Chou
> >
> >
>
--
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU