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 branch name into commit textarea placeholder #6156

Merged
merged 1 commit into from
Sep 25, 2020
Merged

add branch name into commit textarea placeholder #6156

merged 1 commit into from
Sep 25, 2020

Conversation

xieyuanhuata
Copy link
Contributor

Signed-off-by: xieyuanhu xieyuanhuata@163.com

What it does

fix #6114 update commit textarea placeholder to include branch name

How to test

1、pull this commit and start the theia;
2、open 'Source control :Git';

118

Review checklist

Reminder for reviewers

@akosyakov
Copy link
Member

@vince-fugnitto Should it be updated when a branch is changed?

@xieyuanhuata
Copy link
Contributor Author

@akosyakov
ok

@vince-fugnitto
Copy link
Member

@vince-fugnitto Should it be updated when a branch is changed?

Yes, I believe so. I'd make sense to adjust the placeholder whenever a branch is switched.
Example: VSCode

  1. Initially my branch is master:
Screen Shot 2019-09-11 at 8 50 48 AM
  1. Switch to GH-4162 (the placeholder is updated)
Screen Shot 2019-09-11 at 8 51 06 AM

@vince-fugnitto
Copy link
Member

@xieyuanhuata I believe you can update the logic here to build the entire message:

https://github.com/theia-ide/theia/blob/98f12a9fcc3a43c2b5d224e186f871e66a1e11b7/packages/scm/src/browser/scm-widget.tsx#L185-L196

This will allow you to easily call update as well whenever a repository is changed.
I'm not sure it's possible to do it at the moment where you registerScmProviders.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

It also does not work properly on macOS, the following keybinding is displayed incorrectly:

Screen Shot 2019-09-11 at 9 38 54 AM

@xieyuanhuata
Copy link
Contributor Author

@vince-fugnitto
Yes, you are right.I will modify the code.

this.scmService.registerScmProvider(provider, {
input: {
placeholder: 'Message (press {0} to commit)',
placeholder: `Message (${OS.type() === OS.Type.OSX ? '⌘' : 'press '}{0} to commit ${branch ? 'on \'' + branch.name + '\'' : ''})`,
Copy link
Member

@akosyakov akosyakov Sep 12, 2019

Choose a reason for hiding this comment

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

one should use Keybindings.acceleratorFor to create a human readable representation of a keybinding

@svenefftinge
Copy link
Contributor

@xieyuanhuata would be great to have this change! Are you going to update the PR?

@westbury
Copy link
Contributor

I have pushed a second commit that resolves the outstanding issues on this PR.

  • The Keybindings.acceleratorFor was already being used, substituted into {0}. I believe the '⌘Cmd' problem must be a pre-existing bug not related to this issue, but I don't have a Mac.
  • We are limited in the way we listen to changes to the branch because we must stick to the VS Code API. That API abstracts out any concept of branches, but we get the placeholder text from the builtin. To get the placeholder text to work with the builtin, the changes in scm-commit-widget are required. The changes to the two git files bring Theia's git package into alignment with the VS Code API.

@vince-fugnitto vince-fugnitto added the scm issues related to the source control manager label Sep 23, 2020
@vince-fugnitto
Copy link
Member

@westbury would you mind resolving the conflict and squashing the commits? (I can do so as well if necessary)
I will perform a review :)

@westbury
Copy link
Contributor

@vince-fugnitto I can't squash the two commits because I am adding to the original pull-request so two authors. The two commits should be merged separately. I have however rebased. I was not sure I would be able to force-push to someone else's fork, but it seemed to allow me.

@vince-fugnitto
Copy link
Member

@vince-fugnitto I can't squash the two commits because I am adding to the original pull-request so two authors. The two commits should be merged separately. I have however rebased. I was not sure I would be able to force-push to someone else's fork, but it seemed to allow me.

I was thinking one commit with multiple authors or co-authors since logically the pull-request should not be broken down and if we go back in history one of the commits would not be complete. The pull-request as is still has conflicts.

It would also be possible to open a new pull-request and add @xieyuanhuata as a co-author (but the pull-request comes from your branch).

@westbury
Copy link
Contributor

I was thinking one commit with multiple authors or co-authors

I don't think the Eclipse IP checker can process such merged commits. See for example #4279 which was merged as four commits due to multiple authors, and see comment #4279 (comment) by @akosyakov . The e-mail in the commit must be match both the sign-off and the Foundation's records, and there can only be one author e-mail per commit.

The pull-request as is still has conflicts.

Sorry, pushed to wrong remote.

@vince-fugnitto
Copy link
Member

I don't think the Eclipse IP checker can process such merged commits. See for example #4279 which was merged as four commits due to multiple authors, and see comment #4279 (comment) by @akosyakov . The e-mail in the commit must be match both the sign-off and the Foundation's records, and there can only be one author e-mail per commit.

I see! In our case if one helped out with a pull-request we generally do the following a commit: https://github.com/eclipse-theia/theia/pull/8342/commits. Of course only the author will have the credit for GitHub.

I'll leave it up to your discretion however :) I'll perform a review shortly.

@westbury
Copy link
Contributor

@vince-fugnitto You're right. It looks like Eclipse do allow Co-authored-by. I've merged the commits.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@westbury the changes work very well, thank you for taking care of the issue 👍
I verified the feature using both @theia/git and the builtins, with single and multi-repository workspaces.

Co-authored-by: Nigel Westbury <nigelipse@miegel.org>
Signed-off-by: xieyuanhu <xieyuanhuata@163.com>
@westbury westbury merged commit 1075dde into eclipse-theia:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git issues related to git scm issues related to the source control manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[scm|git] update commit textarea placeholder to include branch name
5 participants