Withdrawn: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

On Thu, 9 Apr 2020 09:37:30 GMT, Craig Cavanaugh 
 wrote:

> https://bugs.openjdk.java.net/browse/JDK-8129123
> 
> This pull request fixes JDK-8129123. This bug also effects Windows and Linux 
> platforms.
> Also, I believe JDK-8196037 is a duplicate of this issue.
> 
> I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and 
> Windows 10.
> 
> Thanks!

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jfx/pull/166

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

On Fri, 17 Apr 2020 10:42:30 GMT, Jeanette Winzenburg  
wrote:

> repeating my comment from the [previous pull 
> request](https://github.com/openjdk/jfx/pull/136#issuecomment-608401086):
> I don't think this is yet ready for a technical review - there are some more 
> basic questions that are not yet answered
> :)

> - is it really a bug or a nice-to-have enhancement? couldn't find an example 
> in win, didn't try too hard and nowadays,
>   such plain combos are not a really frequent
>  - while none of the virtualized controls (nor ChoiceBox) combines selection 
> with scrolling to the selected item. For
>combo and choice, I see no reason not make it the default behavior. We 
> need to make certain it behaves "naturally" when
>navigating in the open popup.
>  - instead of catching every list.select (and not forget the unselect) we 
> might consider doing it in a showing handler
>  - alternatively, we might consider to go deeper and support it directly in 
> the listView

- For me / my users / and the open bug, it is a bug due to the current behavior 
being unexpected.  It creates the
  illusion of a preselected value not actually being selected because it's not 
visible if the list is large and has been
  shown.  It creates doubt and the user has to scroll to reconfirm their 
selection which takes extra unnecessary effort
  and time.

- With my testing, for the ComboBox, behavior was smooth and natural.  I've not 
made any attempt to change selection with
  it shown and I'm not certain it can happen unless done programmatically.  
User selection within the list requires
  scrolling, so the selected value is already visible.

- I'm not opposed to taking this approach.  My current work around by extending 
ComboBox is based on scrolling when the
  value is set (restored) programmatically.  I could see how it may be more 
efficient if multiple selections were being
  performed programmatically, but not sure why someone would write code this 
way.  I would think it's a one shot process
  to select the final value.

- Implementing the change in ListView would not change/improve ChoiceBox simply 
because it does not use an underlying
  ListView.   My search on uses of ListView only reviled ComboBox other than 
itself.  Since ListView by itself is not
  collapsed/hidden for typical uses, would automatic scrolling within ListView 
create a confusing experience?

-

PR: https://git.openjdk.java.net/jfx/pull/166

Re: [Rev 01] RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

On Fri, 17 Apr 2020 10:38:06 GMT, Ajit Ghaisas  wrote:

>> Craig Cavanaugh has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Update unit test for JDK_8129123 to check midpoint of the list
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java
>  line 2097:
> 
>> 2096: first <= index  && index <= last);
>> 2097:
>> 2098: index = LIST_SIZE - 1;
> 
> You have tested first and last index selection.
> It will be nice to have a case where we select index = LIST_SIZE/2

Agreed, mid point test added

-

PR: https://git.openjdk.java.net/jfx/pull/166

Re: [Rev 01] RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

> https://bugs.openjdk.java.net/browse/JDK-8129123
> 
> This pull request fixes JDK-8129123. This bug also effects Windows and Linux 
> platforms.
> Also, I believe JDK-8196037 is a duplicate of this issue.
> 
> I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and 
> Windows 10.
> 
> Thanks!

Craig Cavanaugh has updated the pull request incrementally with one additional 
commit since the last revision:

  Update unit test for JDK_8129123 to check midpoint of the list

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/166/files
  - new: https://git.openjdk.java.net/jfx/pull/166/files/28b25370..0b6683dd

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/166/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/166/webrev.00-01

  Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/166.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/166/head:pull/166

PR: https://git.openjdk.java.net/jfx/pull/166

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

On Tue, 14 Apr 2020 11:11:13 GMT, Kevin Rushforth  wrote:

>> Thank you.  I've updated the pull request comments with what I thought was
>> the necessary changes, but jcheck is still failing with
>> 
>> "the commits in this PR have inconsistent user names and/or email
>> addresses. Please amend the commits."
>> 
>> I'm not sure what to do with this error.
>> 
>> Thanks,
>> Craig
>> 
>> On Tue, Apr 14, 2020 at 5:04 AM Ajit Ghaisas 
>> wrote:
>> 
>>> I am taking a look at your changes now.
>>> @ccavanaugh  can you make jcheck pass
>>> while I review. Please see the details of the failing jcheck.
>>>
>>> —
>>> You are receiving this because you were mentioned.
>>> Reply to this email directly, view it on GitHub
>>> , or
>>> unsubscribe
>>> 
>>> .
>>>
>
> I think you will need to rebase and force push your branch.

Seems I've made a further mess attempting to rebase.  I apologize as I know
the list is not intended to be tech support for git usage, but I'm at a
loss and don't want to make it worse.

Below is the log after an interactive rebase and forced push.

""
Merge branch '8129123' of https://github.com/ccavanaugh/jfx into 8129123

branches 8129123, origin/8129123
""

Thanks,
Craig

On Tue, Apr 14, 2020 at 7:11 AM Kevin Rushforth 
wrote:

> I think you will need to rebase and force push your branch.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> 
> .
>

-

PR: https://git.openjdk.java.net/jfx/pull/166

RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

https://bugs.openjdk.java.net/browse/JDK-8129123

This pull request fixes JDK-8129123. This bug also effects Windows and Linux 
platforms.
Also, I believe JDK-8196037 is a duplicate of this issue.

I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and Windows 
10.

Thanks!

-

Commit messages:
 - Fix for JDK-8129123

Changes: https://git.openjdk.java.net/jfx/pull/166/files
 Webrev: https://webrevs.openjdk.java.net/jfx/166/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8129123
  Stats: 51 lines in 2 files changed: 49 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/166.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/166/head:pull/166

PR: https://git.openjdk.java.net/jfx/pull/166

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

On Thu, 16 Apr 2020 12:30:20 GMT, Kevin Rushforth  wrote:

>> Seems I've made a further mess attempting to rebase.  I apologize as I know
>> the list is not intended to be tech support for git usage, but I'm at a
>> loss and don't want to make it worse.
>> 
>> Below is the log after an interactive rebase and forced push.
>> 
>> ""
>> Merge branch '8129123' of https://github.com/ccavanaugh/jfx into 8129123
>> 
>> branches 8129123, origin/8129123
>> ""
>> 
>> Thanks,
>> Craig
>> 
>> On Tue, Apr 14, 2020 at 7:11 AM Kevin Rushforth 
>> wrote:
>> 
>>> I think you will need to rebase and force push your branch.
>>>
>>> —
>>> You are receiving this because you were mentioned.
>>> Reply to this email directly, view it on GitHub
>>> , or
>>> unsubscribe
>>> 
>>> .
>>>
>
> The following should work:
> 
> $ git rebase -i master
> 
> If the rebase is successful, it will pop you into your EDITOR. You then 
> select "pick" for the first commit (which is
> already selected by default) and "s"quash or "f"ixup for all subsequent 
> commits. Then, if it looks good to you, do the
> following:  $ git push --force origin 8129123

That did it!

Thanks!

On Thu, Apr 16, 2020 at 8:30 AM Kevin Rushforth 
wrote:

> The following should work:
>
> $ git rebase -i master
>
> If the rebase is successful, it will pop you into your EDITOR. You then
> select "pick" for the first commit (which is already selected by default)
> and "s"quash or "f"ixup for all subsequent commits. Then, if it looks good
> to you, do the following:
>
> $ git push --force origin 8129123
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> 
> .
>

-

PR: https://git.openjdk.java.net/jfx/pull/166

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

On Tue, 14 Apr 2020 09:04:40 GMT, Ajit Ghaisas  wrote:

>> was: https://github.com/openjdk/jfx/pull/136
>
> I am taking a look at your changes now.
> @ccavanaugh can you make jcheck pass while I review. Please see the details 
> of the failing jcheck.

Thank you.  I've updated the pull request comments with what I thought was
the necessary changes, but jcheck is still failing with

"the commits in this PR have inconsistent user names and/or email
addresses. Please amend the commits."

I'm not sure what to do with this error.

Thanks,
Craig

On Tue, Apr 14, 2020 at 5:04 AM Ajit Ghaisas 
wrote:

> I am taking a look at your changes now.
> @ccavanaugh  can you make jcheck pass
> while I review. Please see the details of the failing jcheck.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> 
> .
>

-

PR: https://git.openjdk.java.net/jfx/pull/166

Re: [Rev 02] RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

> This pull request fixes JDK-8129123.  This bug also effects Windows and Linux 
> platforms.
> Also, I believe JDK-8196037 is a duplicate of this issue.
> 
> I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and 
> Windows 10.
> 
> Thanks!

Craig Cavanaugh has refreshed the contents of this pull request, and previous 
commits have been removed. The
incremental views will show differences compared to the previous content of the 
PR.

This pull request has been closed without being integrated.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/136/files
  - new: https://git.openjdk.java.net/jfx/pull/136/files/229a7fd2..159f6516

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/136/webrev.02
 - incr: https://webrevs.openjdk.java.net/jfx/136/webrev.01-02

  Stats: 51 lines in 2 files changed: 0 ins; 49 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/136.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/136/head:pull/136

PR: https://git.openjdk.java.net/jfx/pull/136

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

My use case which is driving the fix for this bug is user expectation in an
open source application (https://github.com/ccavanaugh/jgnash).

When a user makes an edit to an existing transaction, the expectation is
the prior selected ComboBox value is visible when the list is displayed,
otherwise it gives the impression they made an error or something is
wrong.  I think the root cause of the poor experience is seeing the correct
value when collapsed, and then not seeing it or it's selection when
expanded.

Also, scrolling automatically was the default behavior for Swing.

My current work around is an extended ComboBox that listens for a mouse
click or key events and then scrolls the list view.

On Fri, Apr 3, 2020 at 8:19 AM Jeanette Winzenburg <
faste...@openjdk.java.net> wrote:

> On Fri, 3 Apr 2020 12:03:33 GMT, Jeanette Winzenburg 
> wrote:
>
> >>> Can you please provide a unit test? One that fails before your fix and
> passes after your fix.
> >>
> >> I can provide a manual test the next couple of days that demonstrates
> it before and after, but I'm not sure how to
> >> create an automated unit test because the issue is visual.
> >
> > A quick snippet of how-to write a unit test (for setup see other tests
> in controls) that will fail before and  pass
> > after the change:
> > @Test
> > public void testScrollInSkin() {
> > int index = 50;
> > comboBox.getSelectionModel().select(index);
> > comboBox.show();
> > VirtualFlow> virtualFlow = getFlow();
> > int first = virtualFlow.getFirstVisibleCell().getIndex();
> > int last = virtualFlow.getLastVisibleCell().getIndex();
> > assertTrue(" visible range [" + first + ", " + last + "] must
> include " + index,
> > first <= index  && index <= last);
> > }
> >
> > protected VirtualFlow> getFlow() {
> > VirtualFlow> virtualFlow =
> (VirtualFlow>)
> > VirtualFlowTestUtils.getVirtualFlow(comboBox);
> > return virtualFlow;
> > }
>
> A couple of comments to the bug (and fix) itself:
>
> - is it really a bug or a nice-to-have enhancement? couldn't find an
> example in win, didn't try too hard and nowadays,
>   such plain combos are not a really frequent
> - while none of the virtualized controls (nor ChoiceBox) combines
> selection with scrolling to the selected item. For
>   combo and choice, I see no reason not make it the default behavior. We
> need to make certain it behaves "naturally" when
>   navigating in the open popup. instead of catching every list.select (and
> not forget the unselect) we might consider
>   doing it in a showing handler  alternatively, we might consider to go
> deeper and support it directly in the listView
>
> -
>
> PR: https://git.openjdk.java.net/jfx/pull/136
>

Re: [Rev 01] RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

> This pull request fixes JDK-8129123.  This bug also effects Windows and Linux 
> platforms.
> Also, I believe JDK-8196037 is a duplicate of this issue.
> 
> I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and 
> Windows 10.
> 
> Thanks!

Craig Cavanaugh has updated the pull request incrementally with one additional 
commit since the last revision:

  Unit test for JDK_8129123

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/136/files
  - new: https://git.openjdk.java.net/jfx/pull/136/files/8e06c9e1..229a7fd2

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/136/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/136/webrev.00-01

  Stats: 47 lines in 1 file changed: 46 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/136.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/136/head:pull/136

PR: https://git.openjdk.java.net/jfx/pull/136

Re: Stuck attempting to run/develop a manual unit test

[Craig Cavanaugh]

Jeanette,
Thank you!  Much appreciated and a much better approach than a manual test.

Well on my way now!

Thanks,
Craig

On Fri, Apr 3, 2020 at 7:48 AM Jeanette Winzenburg 
wrote:

>
> Hi Craig,
>
> no manual test needed - you can implement a normal (unit) test with
> the help or VirtualFlowTestUtils: grab the flow, get its first/last
> visible cell and check whether their range includes to cell we want to
> see, something like:
>
>  @Test
>  public void testScrollInSkin() {
>  int index = 50;
>  comboBox.getSelectionModel().select(index);
>  comboBox.show();
>  VirtualFlow> virtualFlow = getFlow();
>  int first = virtualFlow.getFirstVisibleCell().getIndex();
>  int last = virtualFlow.getLastVisibleCell().getIndex();
>  assertTrue(" visible range [" + first + ", " + last + "] must
> include " + index,
>  first <= index  && index <= last);
>  }
>
>  protected VirtualFlow> getFlow() {
>  VirtualFlow> virtualFlow =
> (VirtualFlow>)
>  VirtualFlowTestUtils.getVirtualFlow(comboBox);
>  return virtualFlow;
>      }
>
> Have a look at the tests in test.xx.controls to understand how to
> setup the test context. And have fun :)
>
> Zitat von Craig Cavanaugh :
>
> > I am trying to create a manual unit test for RFR: 8129123 and I'm using
> the
> > existing ButtonMnemonicPositionTest.java as a template to make sure
> > everything is working correctly.
> >
> > I'm using an Ubuntu 18.04.4 build environment using the default 11.0.6
> > openjdk.
> > openjfx is successfully compiling and passing tests following the wiki
> > instructions
> >
> > I'm able to compile ButtonMnemonicPositionTest successfully as shown
> below.
> > javac -classpath  .:/home/craig/IdeaProjects/jfx/build/sdk/lib/*
> > ButtonMnemonicPositionTest.java
> >
> > But attempts to run are failing despite a successful compile:
> > java -classpath  .:/home/craig/IdeaProjects/jfx/build/sdk/lib/*
> > ButtonMnemonicPositionTest
> > Error: JavaFX runtime components are missing, and are required to run
> this
> > application
> >
> > I've also tried running using the modular-sdk after a Gradle build and no
> > luck either after finding a recommendation on the list that the module
> > approach should be used for testing.
> >
> > java --module-path /home/craig/IdeaProjects/jfx/build/modular-sdk/modules
> > --add-modules=javafx.controls ButtonMnemonicPositionTest
> > Error occurred during initialization of boot layer
> > java.lang.module.FindException: Module javafx.controls not found
> >
> > I've been search through the list archive and haven't found a solution.
> > I'm sure I'm doing something wrong.
> >
> > Some help please!
> >
> > Regards,
> > Craig
>
>
>
>

Re: Stuck attempting to run/develop a manual unit test

[Craig Cavanaugh]

Thanks!  That was the trick!

On Fri, Apr 3, 2020 at 6:52 AM Ajit Ghaisas  wrote:

> Hi Craig,
>
> Here is how I run the ButtonMnemonicPositionTest.
>
> $ export FX_LIB=/Users/AG/aaa/jfx/build/sdk/lib
> $ cd /Users/AG/aaa/jfx/tests/manual/UI
> $ javac --module-path $FX_LIB --add-modules javafx.controls
> ButtonMnemonicPositionTest.java
> $ java --module-path $FX_LIB --add-modules javafx.controls
> ButtonMnemonicPositionTest
>
> Hope this helps.
>
> We can discuss further on your PR.
>
> Regards,
> Ajit
>
>
> > On 03-Apr-2020, at 3:56 PM, Craig Cavanaugh 
> wrote:
> >
> > I am trying to create a manual unit test for RFR: 8129123 and I'm using
> the
> > existing ButtonMnemonicPositionTest.java as a template to make sure
> > everything is working correctly.
> >
> > I'm using an Ubuntu 18.04.4 build environment using the default 11.0.6
> > openjdk.
> > openjfx is successfully compiling and passing tests following the wiki
> > instructions
> >
> > I'm able to compile ButtonMnemonicPositionTest successfully as shown
> below.
> > javac -classpath  .:/home/craig/IdeaProjects/jfx/build/sdk/lib/*
> > ButtonMnemonicPositionTest.java
> >
> > But attempts to run are failing despite a successful compile:
> > java -classpath  .:/home/craig/IdeaProjects/jfx/build/sdk/lib/*
> > ButtonMnemonicPositionTest
> > Error: JavaFX runtime components are missing, and are required to run
> this
> > application
> >
> > I've also tried running using the modular-sdk after a Gradle build and no
> > luck either after finding a recommendation on the list that the module
> > approach should be used for testing.
> >
> > java --module-path /home/craig/IdeaProjects/jfx/build/modular-sdk/modules
> > --add-modules=javafx.controls ButtonMnemonicPositionTest
> > Error occurred during initialization of boot layer
> > java.lang.module.FindException: Module javafx.controls not found
> >
> > I've been search through the list archive and haven't found a solution.
> > I'm sure I'm doing something wrong.
> >
> > Some help please!
> >
> > Regards,
> > Craig
>
>

Stuck attempting to run/develop a manual unit test

[Craig Cavanaugh]

I am trying to create a manual unit test for RFR: 8129123 and I'm using the
existing ButtonMnemonicPositionTest.java as a template to make sure
everything is working correctly.

I'm using an Ubuntu 18.04.4 build environment using the default 11.0.6
openjdk.
openjfx is successfully compiling and passing tests following the wiki
instructions

I'm able to compile ButtonMnemonicPositionTest successfully as shown below.
javac -classpath  .:/home/craig/IdeaProjects/jfx/build/sdk/lib/*
ButtonMnemonicPositionTest.java

But attempts to run are failing despite a successful compile:
java -classpath  .:/home/craig/IdeaProjects/jfx/build/sdk/lib/*
ButtonMnemonicPositionTest
Error: JavaFX runtime components are missing, and are required to run this
application

I've also tried running using the modular-sdk after a Gradle build and no
luck either after finding a recommendation on the list that the module
approach should be used for testing.

java --module-path /home/craig/IdeaProjects/jfx/build/modular-sdk/modules
--add-modules=javafx.controls ButtonMnemonicPositionTest
Error occurred during initialization of boot layer
java.lang.module.FindException: Module javafx.controls not found

I've been search through the list archive and haven't found a solution.
I'm sure I'm doing something wrong.

Some help please!

Regards,
Craig

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

On Thu, 2 Apr 2020 22:20:18 GMT, Kevin Rushforth  wrote:

> Can you please provide a unit test? One that fails before your fix and passes 
> after your fix.

I can provide a manual test the next couple of days that demonstrates it before 
and after, but I'm not sure how to
create an automated unit test because the issue is visual.

-

PR: https://git.openjdk.java.net/jfx/pull/136

RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

This pull request fixes JDK-8129123.  This bug also effects Windows and Linux 
platforms.
Also, I believe JDK-8196037 is a duplicate of this issue.

I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and Windows 
10.

Thanks!

-

Commit messages:
 - Merge branch 'master' of https://github.com/openjdk/jfx
 - Merge branch 'master' of https://github.com/openjdk/jfx
 - Fix for JDK-8129123

Changes: https://git.openjdk.java.net/jfx/pull/136/files
 Webrev: https://webrevs.openjdk.java.net/jfx/136/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8129123
  Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/136.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/136/head:pull/136

PR: https://git.openjdk.java.net/jfx/pull/136