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

Sync up with changes from voting branch #1601

Merged
merged 1 commit into from
Jul 11, 2018
Merged

Sync up with changes from voting branch #1601

merged 1 commit into from
Jul 11, 2018

Conversation

ManfredKarrer
Copy link
Contributor

There have been several changes in the voting branch (and api-integration) which I want to get into master to avoid that those branches run out of sync as well as to apply the improvements.
The main changes are:

  • The handling of persisted data in the p2p library (Change data persitence  bisq-p2p#8)
  • Improvements of the app setup to support API projects in headless mode as well as in mixed mode
  • Use custom TxBroadcaster class for better error handling

Beside that there are many smaller trivial improvements, fixes, refactoring and cleanups.

Changes in that project:

  • Add onUiReadyHandler to BisqApp and MainView
  • Add startApplication and onApplicationStarted to BisqAppMain
  • Improve MenuItem component
  • Use TxBroadcaster.Callback() instead of FutureCallback()
  • Apply startup changes to MainViewModel
  • Apply changes of data storage

- Add onUiReadyHandler to BisqApp and MainView
- Add startApplication and onApplicationStarted to BisqAppMain
- Improve MenuItem component
- Use TxBroadcaster.Callback() instead of FutureCallback<Transaction>()
- Apply startup changes to MainViewModel
- Apply changes of data storage
@ManfredKarrer ManfredKarrer requested a review from sqrrm July 8, 2018 11:54
@ripcurlx
Copy link
Contributor

ripcurlx commented Jul 9, 2018

ACK - checked out all branches locally and did a test trade. Everything working so far for me.

@ManfredKarrer
Copy link
Contributor Author

So the ACK applies to all sub projects?

@@ -159,7 +153,8 @@ public void initialize() {
bsqFormatter.formatCoinWithCode(receiverAmount)))
.actionButtonText(Res.get("shared.yes"))
.onAction(() -> {
walletsManager.publishAndCommitBsqTx(txWithBtcFee, new FutureCallback<Transaction>() {
// TODO not updated to TxBroadcaster changes because not in sync with voting branch anyway
/* walletsManager.publishAndCommitBsqTx(txWithBtcFee, new FutureCallback<Transaction>() {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be fixed now that voting branch is being merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I did not merge the dao code, just a few parts which was required due the refactoring. So the dao code in master is still the old one from a few months back...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A main reason why not merge yet is bc of PB changes need to be tested well and if we have it in master it will be harder to change, so better for the WIP state to be more flexible with PB changes....

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, noticing parts are there but a lot isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, only technical refactorings (class rename, move) in the dao project got applied.

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

@ripcurlx
Copy link
Contributor

@ManfredKarrer I checked out all PR's and built the client. So my testing involved the other PR's as well.

@cbeams
Copy link
Contributor

cbeams commented Jul 11, 2018

Same question here as in #1605 (comment). Was it unintentional to merge this as @bisq-github-admin-2?

@ManfredKarrer
Copy link
Contributor Author

Yes because I did not had merge rights anymore with my dev account. I merged already the other projects so it would have caused problems to leave desktop with the old version.

@cbeams
Copy link
Contributor

cbeams commented Jul 11, 2018

@ManfredKarrer and I sorted out the merge permissions issue. See #1605 (comment) for details.

@ManfredKarrer ManfredKarrer deleted the sync-with-voting branch August 14, 2018 19:01
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