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

[INTERNAL][UI] Use new XMPPStore API #609

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

srossbach
Copy link
Contributor

This patch introduces the usage of the new XMPPStore API.

Renaming UI buttons is not done yet as changing UI values is likely to
crash the whole STF.

@srossbach srossbach changed the base branch from master to account_store July 31, 2019 13:26
@srossbach srossbach requested a review from m273d15 July 31, 2019 13:28
@tobous
Copy link
Member

tobous commented Aug 2, 2019

@m273d15 Any idea why the Codacy build fails? I tried restarting it, but it did not help. Otherwise, I will contact the codacy support.

@srossbach Did you change the base of this PR directly after pushing to the PR branch (while the build was still running)? This caused issue for me in the past.

@srossbach
Copy link
Contributor Author

I do not know what cause this. I am already facing the problem that my commit message is not insert here. I will have to look at EGIT again if it is possible to add a reference on a remote branch. Currently I am doing this in Github when creating the pull request. Maybe it is possible that I changed the branch during build.

@tobous
Copy link
Member

tobous commented Aug 2, 2019

What do you mean by "that your commit message is not inserted here"? Do you mean that the commit message is actually not attached to the commit or that the commit message is not automatically entered in the PR description?

If its the second one, this is due to us now having a PR template that is used instead. So you have to do the same you also have to do when creating PRs containing multiple commits, i.e. copy the message and put it in the PR description by hand. At least that is how I have been doing it. Maybe there is a better way or a plugin that helps you with it, but this would be news to me.

@srossbach
Copy link
Contributor Author

Sometimes my commit message is included in the PR and the template is appended. Sometimes only the template is present.

Copy link
Member

@tobous tobous left a comment

Choose a reason for hiding this comment

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

Ok, either I am missing something or there are a lot of potential NPEs in the connection logic with the new change to allow no default account to be set. It seems to often have been used as a general fallback, which is now problematic.

@srossbach srossbach force-pushed the pr/use_new_accountstore_api branch from 627d047 to a6f20a1 Compare August 2, 2019 20:45
@srossbach srossbach force-pushed the pr/use_new_accountstore_api branch from a6f20a1 to 362e2b6 Compare August 2, 2019 22:55
@srossbach srossbach force-pushed the account_store branch 2 times, most recently from 4e5b296 to 502c6ab Compare August 5, 2019 18:07
tobous
tobous previously approved these changes Aug 5, 2019
Copy link
Member

@tobous tobous left a comment

Choose a reason for hiding this comment

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

Needs to be rebased and there is still my question about a possible NPE. But looks good otherwise.

@srossbach
Copy link
Contributor Author

I just noticed I missed the server. Will add it in the next commit.

This patch introduces the usage of the new XMPPStore API.

Renaming UI buttons is not done yet as changing UI values is likely to
crash the whole STF.
@srossbach srossbach force-pushed the pr/use_new_accountstore_api branch from 362e2b6 to e4ba631 Compare August 5, 2019 18:55
@srossbach srossbach changed the title [INTERNAL][UI][E] Use new XMPPStore API [INTERNAL][UI] Use new XMPPStore API Aug 5, 2019
@saros-project saros-project deleted a comment Aug 5, 2019
@srossbach srossbach changed the base branch from account_store to master August 5, 2019 19:00
Copy link
Member

@tobous tobous left a comment

Choose a reason for hiding this comment

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

Looks good to me. Did not check whether there are any other usages of the old (now deprecated) methods.

@srossbach
Copy link
Contributor Author

Should be easily revealed when rebasing #615 .

@tobous
Copy link
Member

tobous commented Aug 5, 2019

@srossbach From my experience, the Travis build for the Pull Request will never come in (in the current state). This is an issue that seems to occur when (force-) pushing to a branch and then immediately changing the base branch while it is still building.

This can only be solved by pushing something new to the branch (i.e. amending a commit and force-push again). But, since the branch build succeeded, this can probably be ignored. It should still be avoided if possible (e.g. by changing the base branch before pushing/updating the PR).

@srossbach
Copy link
Contributor Author

@tobous thank you for the information. I am still used to Gerrit which updated the branches automatically.

@srossbach srossbach merged commit 8f04b6e into master Aug 5, 2019
@srossbach srossbach deleted the pr/use_new_accountstore_api branch August 5, 2019 20:32
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.

2 participants