Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dropdown autocomplete on Market, Buy, Sell tabs #3139

Conversation

battleofwizards
Copy link
Contributor

@battleofwizards battleofwizards commented Aug 25, 2019

Basic autocomplete feature for all dropdowns on the major tabs:

  • Market / Offer Book
  • Market / Trades
  • Buy BTC
  • Sell BTC

Known limitations:

  • Autocomplete still missing from Settings, Account, DAO tabs
  • Minor UX glitches remain despite really lots of debugging and polishing

Related issues:

Note to reviewers:

The new code is mostly around the SearchComboBox and StringConverters. Other changes are mostly a necessary reshuffling of the existing code around ComboBox setup in the forms.

@battleofwizards battleofwizards force-pushed the add-autocomplete-on-market-buy-sell-tabs branch 3 times, most recently from 6520c7f to bf51533 Compare August 25, 2019 21:22
@battleofwizards
Copy link
Contributor Author

Here is a little teaser, click to watch a movie:

Bisq autocomplete

@ripcurlx
Copy link
Contributor

Here is a little teaser, click to watch a movie:

Bisq autocomplete

You really want to get this PR merged soon 😉

@wiz
Copy link
Contributor

wiz commented Aug 27, 2019

utACK but how can this work when it doesn't add 10 new jar deps? 😂

@ripcurlx
Copy link
Contributor

Just got an exception when I typed into the search combobox in the markets offer book

java.util.NoSuchElementException
	at java.base/java.util.AbstractList$Itr.next(AbstractList.java:377)
	at java.base/java.util.AbstractCollection.toArray(AbstractCollection.java:144)
	at java.base/java.util.ArrayList.<init>(ArrayList.java:179)
	at javafx.collections.transformation.FilteredList.refilter(FilteredList.java:325)
	at javafx.collections.transformation.FilteredList.access$000(FilteredList.java:50)
	at javafx.collections.transformation.FilteredList$1.invalidated(FilteredList.java:102)
	at javafx.beans.property.ObjectPropertyBase.markInvalid(ObjectPropertyBase.java:112)
	at javafx.beans.property.ObjectPropertyBase.set(ObjectPropertyBase.java:147)
	at javafx.collections.transformation.FilteredList.setPredicate(FilteredList.java:125)
	at bisq.desktop.components.SearchComboBox.filterBy(SearchComboBox.java:154)
	at bisq.desktop.components.SearchComboBox.lambda$prepareQueryChangedListener$5(SearchComboBox.java:181)
	at com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:428)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:427)
	at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)

It did work after a restart, but it was weird to see this on the first time. Maybe a sync of the list in the background triggered this weird behavior. I'll give it a little more testing to see if I find the problem.

@ripcurlx
Copy link
Contributor

It would be great if it would work that you select the selected market (e.g. EUR) and just type in your desired market (e.g. XMR) and replace and find it. Atm it writes the search term without the first character in front of the original market (e.g. mrEUR - Euro). It does work afterwards when selecting the text and typing in the desired market again.
After searching and selecting a new market (e.g. XMR) and clicking on the drop down afterwards it will only show the filtered list (in my case only XMR). I think it would be great after selecting the desired market that all markets will be displayed afterwards when clicking on the drop down icon of the combo box. Atm you have to delete the current market to receive the full list of markets again.

@battleofwizards
Copy link
Contributor Author

Thanks a lot @ripcurlx!

I'm mostly worried about your exception b/c I cannot recreate it. Will try more. This needs to be fixed.

I confirm the two UX glitches you mentioned. The ComboBox is half-magic internally and it is being embedded in a complex context. It is very hard to bend it to perfect autocomplete experience.

I already spent much more time fighting this than I ever anticipated. OTOH I hate to deliver crap. I will see what I can do.

@christophsturm
Copy link
Contributor

@ripcurlx with what jdk did you run when you got that stacktrace?

@christophsturm
Copy link
Contributor

another stacktrace:

	at java.base/java.util.AbstractList$Itr.next(AbstractList.java:377)
	at java.base/java.util.Spliterators$IteratorSpliterator.tryAdvance(Spliterators.java:1812)
	at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:127)
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:502)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:488)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.FindOps$FindOp.evaluateSequential(FindOps.java:150)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.findAny(ReferencePipeline.java:548)
	at bisq.desktop.main.market.offerbook.OfferBookChartView$CurrencyListItemStringConverter.fromString(OfferBookChartView.java:307)
	at bisq.desktop.main.market.offerbook.OfferBookChartView$CurrencyListItemStringConverter.fromString(OfferBookChartView.java:290)
	at javafx.scene.control.ComboBox.commitValue(ComboBox.java:460)
	at javafx.scene.control.ComboBox.lambda$new$3(ComboBox.java:292)
	at com.sun.javafx.binding.ExpressionHelper$Generic.fireValueChangedEvent(ExpressionHelper.java:348)
	at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:80)
	at javafx.beans.property.ReadOnlyBooleanPropertyBase.fireValueChangedEvent(ReadOnlyBooleanPropertyBase.java:72)
	at javafx.scene.Node$FocusedProperty.notifyListeners(Node.java:8148)
	at javafx.scene.Node.setFocused(Node.java:8201)
	at javafx.scene.Scene$KeyHandler.setWindowFocused(Scene.java:4026)
	at javafx.scene.Scene$KeyHandler.lambda$new$0(Scene.java:4048)
	at com.sun.javafx.binding.ExpressionHelper$SingleInvalidation.fireValueChangedEvent(ExpressionHelper.java:136)
	at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:80)
	at javafx.beans.property.ReadOnlyBooleanPropertyBase.fireValueChangedEvent(ReadOnlyBooleanPropertyBase.java:72)
	at javafx.beans.property.ReadOnlyBooleanWrapper.fireValueChangedEvent(ReadOnlyBooleanWrapper.java:103)
	at javafx.beans.property.BooleanPropertyBase.markInvalid(BooleanPropertyBase.java:111)
	at javafx.beans.property.BooleanPropertyBase.set(BooleanPropertyBase.java:145)
	at javafx.stage.Window.setFocused(Window.java:674)
	at javafx.stage.Window$1.setFocused(Window.java:149)
	at com.sun.javafx.stage.WindowHelper.setFocused(WindowHelper.java:112)
	at com.sun.javafx.stage.WindowPeerListener.changedFocused(WindowPeerListener.java:64)
	at com.sun.javafx.tk.quantum.GlassWindowEventHandler.run(GlassWindowEventHandler.java:126)
	at com.sun.javafx.tk.quantum.GlassWindowEventHandler.run(GlassWindowEventHandler.java:40)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at com.sun.javafx.tk.quantum.GlassWindowEventHandler.lambda$handleWindowEvent$4(GlassWindowEventHandler.java:176)
	at com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:390)
	at com.sun.javafx.tk.quantum.GlassWindowEventHandler.handleWindowEvent(GlassWindowEventHandler.java:174)
	at com.sun.glass.ui.Window.handleWindowEvent(Window.java:1346)
	at com.sun.glass.ui.Window.notifyFocus(Window.java:1325)
	at com.sun.glass.ui.mac.MacWindow._setVisible(Native Method)
	at com.sun.glass.ui.Window.setVisible(Window.java:689)
	at com.sun.javafx.tk.quantum.WindowStage.lambda$setVisible$2(WindowStage.java:530)
	at com.sun.javafx.tk.quantum.QuantumToolkit.runWithRenderLock(QuantumToolkit.java:408)
	at com.sun.javafx.tk.quantum.WindowStage.setVisible(WindowStage.java:526)
	at javafx.stage.Window$12.invalidated(Window.java:1121)
	at javafx.beans.property.BooleanPropertyBase.markInvalid(BooleanPropertyBase.java:110)
	at javafx.beans.property.BooleanPropertyBase.set(BooleanPropertyBase.java:145)
	at javafx.stage.Window.setShowing(Window.java:1174)
	at javafx.stage.Window.show(Window.java:1189)
	at javafx.stage.Stage.show(Stage.java:273)
	at bisq.desktop.main.overlays.Overlay.display(Overlay.java:540)
	at bisq.desktop.main.overlays.popups.Popup.onReadyForDisplay(Popup.java:32)
	at bisq.desktop.main.overlays.popups.PopupManager.displayNext(PopupManager.java:54)
	at bisq.desktop.main.overlays.popups.PopupManager.queueForDisplay(PopupManager.java:37)
	at bisq.desktop.main.overlays.popups.Popup.onShow(Popup.java:37)
	at bisq.desktop.main.overlays.Overlay.show(Overlay.java:206)
	at bisq.desktop.main.overlays.Overlay.show(Overlay.java:211)
	at bisq.desktop.app.BisqApp.stop(BisqApp.java:157)
	at bisq.desktop.app.BisqApp.shutDownByUser(BisqApp.java:339)
	at bisq.desktop.app.BisqApp.lambda$setupStage$5(BisqApp.java:244)
	at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)
	at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:238)
	at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
	at com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
	at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
	at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
	at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
	at javafx.event.Event.fireEvent(Event.java:198)
	at com.sun.javafx.stage.WindowPeerListener.closing(WindowPeerListener.java:93)
	at com.sun.javafx.tk.quantum.GlassWindowEventHandler.run(GlassWindowEventHandler.java:147)
	at com.sun.javafx.tk.quantum.GlassWindowEventHandler.run(GlassWindowEventHandler.java:40)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at com.sun.javafx.tk.quantum.GlassWindowEventHandler.lambda$handleWindowEvent$4(GlassWindowEventHandler.java:176)
	at com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:390)
	at com.sun.javafx.tk.quantum.GlassWindowEventHandler.handleWindowEvent(GlassWindowEventHandler.java:174)
	at com.sun.glass.ui.Window.handleWindowEvent(Window.java:1346)
	at com.sun.glass.ui.Window.notifyClose(Window.java:1246)```

@battleofwizards battleofwizards changed the title Add dropdown autocomplete on Market, Buy, Sell tabs [WIP] Add dropdown autocomplete on Market, Buy, Sell tabs Aug 27, 2019
@ripcurlx
Copy link
Contributor

ripcurlx commented Aug 27, 2019

The ComboBox is half-magic internally and it is being embedded in a complex context. It is very hard to bend it to perfect autocomplete experience.

Yeah, some of the JavaFX compontents are not really straight forward implemented.

@ripcurlx with what jdk did you run when you got that stacktrace?

I'm using OpenJDK 10 for development.

@ManfredKarrer
Copy link
Contributor

@battleofwizards Congrats that you got so far!!! Beside @ripcurlx I don't know anyone who can master JavaFX component magic.

@battleofwizards battleofwizards force-pushed the add-autocomplete-on-market-buy-sell-tabs branch 2 times, most recently from d8d87bb to 9b8bdb3 Compare August 29, 2019 18:07
@battleofwizards battleofwizards changed the title [WIP] Add dropdown autocomplete on Market, Buy, Sell tabs Add dropdown autocomplete on Market, Buy, Sell tabs Aug 29, 2019
@battleofwizards
Copy link
Contributor Author

battleofwizards commented Aug 29, 2019

The exceptions should now be fixed. The ComboBox was basically rewritten and now works with its own cloned list to avoid cross-thread sharing. The ComboBox list still gets updated on tab activation. Items should be fresh.

The specific UX glitches found by @ripcurlx are worked around. Not strictly fixed but made less relevant. Some UX imperfections remain but I do not feel like I can improve this any further. At least not this time around. My recommendation is to merge as-is b/c it is strict improvement over no autocomplete at all.

Ready for review!

…#2226

Basic autocomplete feature for all dropdowns on the major tabs:

* Market / Offer Book
* Market / Trades
* Buy BTC
* Sell BTC

Known limitations:

* Autocomplete still missing from Settings, Account, DAO tabs
* Minor UX glitches remain despite lots of debugging and polishing

Related issues:

* fix bisq-network#2226
* partially addressed bisq-network#2712
* superseded bisq-network#112
@battleofwizards battleofwizards force-pushed the add-autocomplete-on-market-buy-sell-tabs branch from 9b8bdb3 to 3f1b188 Compare August 29, 2019 18:34
@ripcurlx
Copy link
Contributor

I'll review the PR before the code freeze. 👍

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK - Thanks for the contribution @battleofwizards! It looks like we have a new JavaFX expert 😄

@ripcurlx ripcurlx merged commit 6e232d8 into bisq-network:master Aug 30, 2019
@battleofwizards
Copy link
Contributor Author

Very cool to see this merged, thanks!

Regarding JavaFX though the inconvenient truth is it was a brutal fight and many days going back and forth, rather than my expertise.

@battleofwizards battleofwizards deleted the add-autocomplete-on-market-buy-sell-tabs branch September 20, 2019 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants