-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 indication that BSQ transactions are not up to date #2388
Add indication that BSQ transactions are not up to date #2388
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
I have tested and it works, but I still think it would be better with a separate service for the sync handling to not pollute the setup class.
/** | ||
* High level entry point for Dao domain. | ||
* We initialize all main service classes here to be sure they are started. | ||
*/ | ||
public class DaoSetup { | ||
public class DaoSetup implements DaoStateListener { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to make a separate BSQSyncService or something like that, it feels wrong to add this to the DaoSetup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not use DaoSetup for that but DaoFacade (or some custom class). I will have a look soon into it to make a more concrete sugestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also wasn't quite happy to put it in this place, I just though to make it consistent how it is handled with the btcInfo WalletSetup right now:
bisq/core/src/main/java/bisq/core/app/BisqSetup.java
Lines 341 to 348 in 8cdc759
// Wallet | |
public StringProperty getBtcInfo() { | |
return walletAppSetup.getBtcInfo(); | |
} | |
public DoubleProperty getBtcSyncProgress() { | |
return walletAppSetup.getBtcSyncProgress(); | |
} |
private final StringProperty btcInfo = new SimpleStringProperty(Res.get("mainView.footer.btcInfo.initializing")); |
How do we differentiation between a Facade and a Service atm in Bisq?
I'll have a look to find a better place for this presentation logic. I might change the implementation of the btcInfo/btcSyncProgress on the way as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the code into the DaoPresentation class which I created already for another DAO related PR.
8579fbd
to
d6b1863
Compare
d6b1863
to
6bea5da
Compare
Fixes #2303.