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

Check if selected proposal is null #2636

Conversation

Projects
None yet
2 participants
@ripcurlx
Copy link
Member

commented Apr 4, 2019

Fixes #2627.

@ripcurlx ripcurlx requested a review from ManfredKarrer as a code owner Apr 4, 2019

@ManfredKarrer
Copy link
Member

left a comment

utACK

@ManfredKarrer

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@ripcurlx Could you reproduce the issue? Can it be that the user clicks on the icon in the table but no proposal is selected and therefore no voting will be applied? I am not sure if the icon would reflect correctly the domain state or if it gets changed by the mouse click. If so there might be a risk that the domain state is different as the UI state...

@ripcurlx

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@ripcurlx Could you reproduce the issue? Can it be that the user clicks on the icon in the table but no proposal is selected and therefore no voting will be applied? I am not sure if the icon would reflect correctly the domain state or if it gets changed by the mouse click. If so there might be a risk that the domain state is different as the UI state...

I just found the problem (block parsing while the popup is open) and will implement it in a better way (update the open popup)

@ManfredKarrer

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Ah yes with a new block the table gets updated and selection is lost... that is annoying anyway but have not found a easy fix for that.... maybe to not clear selction would be first step.

@ripcurlx

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Ah yes with a new block the table gets updated and selection is lost... that is annoying anyway but have not found a easy fix for that.... maybe to not clear selction would be first step.

To not mess too much with the existing UI code I'm closing and re-opening the selected proposal window after a new block has been parsed by selecting the same proposal if it still exists.

@ManfredKarrer
Copy link
Member

left a comment

utACK

Should be tested good. Not 100% sure if the handling of the selction/hide is covering all situations correctly from the timing behaviour...

@ripcurlx

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

utACK

Should be tested good. Not 100% sure if the handling of the selction/hide is covering all situations correctly from the timing behaviour...

For that I was using the onHidden trigger. Worked for me now during testing except after having a proposal window open until the next cycle starts because of the current logic (doesn't break anything if it is closed). If we want to be on the 100% safe side, we could just hide the window after a new block was parsed.

@ManfredKarrer

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I will test as well... please wait with merge a bit

@ManfredKarrer

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@ripcurlx Tested more and seems all Ok. But found another bug.
Could you apply that patch?

IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- desktop/src/main/java/bisq/desktop/main/dao/governance/proposals/ProposalsView.java	(date 1554388467000)
+++ desktop/src/main/java/bisq/desktop/main/dao/governance/proposals/ProposalsView.java	(date 1554394709000)
@@ -204,6 +204,8 @@
 
         selectedProposalSubscription = EasyBind.subscribe(tableView.getSelectionModel().selectedItemProperty(), this::onSelectProposal);
 
+        daoFacade.addBsqStateListener(this);
+
         sortedList.comparatorProperty().bind(tableView.comparatorProperty());
         tableView.setPrefHeight(100);
         root.getScene().heightProperty().addListener(sceneHeightListener);
@@ -304,7 +306,7 @@
     private void addListenersAfterParseBlockChainComplete() {
         daoFacade.getActiveOrMyUnconfirmedProposals().addListener(proposalListChangeListener);
         daoFacade.getAllBallots().addListener(ballotListChangeListener);
-        daoFacade.addBsqStateListener(this);
+
         bsqWalletService.addBsqBalanceListener(this);
 
         phaseSubscription = EasyBind.subscribe(daoFacade.phaseProperty(), this::onPhaseChanged);
@@ -314,10 +316,6 @@
         listItems.forEach(ProposalsListItem::cleanup);
         listItems.clear();
 
-        fillListItems();
-    }
-
-    private void fillListItems() {
         if (daoFacade.phaseProperty().get().ordinal() < DaoPhase.Phase.BLIND_VOTE.ordinal()) {
             // proposal phase
             List<Proposal> list = daoFacade.getActiveOrMyUnconfirmedProposals();
@ripcurlx

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@ManfredKarrer Just applied your patch

@ManfredKarrer ManfredKarrer merged commit 4e173ac into bisq-network:master Apr 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.