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

CHE-3271: Add more Git actions to context menu #3618

Merged
merged 8 commits into from
Jan 10, 2017
Merged

CHE-3271: Add more Git actions to context menu #3618

merged 8 commits into from
Jan 10, 2017

Conversation

vinokurig
Copy link
Contributor

What does this PR do?

Added file related git actions to project explorer and editor context menu.
git context menu

Updated those git actions to have an ability to use them with resource selection.

What issues does this PR fix or reference?

#3271

Tests written?

Yes

Resource[] resources = appContext.getResources();
Path[] paths = new Path[resources.length];
for (int i = 0; i < resources.length; i++) {
Path path = resources[i].getLocation().removeFirstSegments(appContext.getRootProject().getLocation().segmentCount());
Copy link
Contributor

Choose a reason for hiding this comment

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

appContext.getRootProject().getLocation().segmentCount() will be always equals to 1, so you can safely remove the first segment without this checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

**/
@Singleton
public class AddToIndexAction extends GitAction {
private final AddToIndexPresenter presenter;
private static final String ADD_TO_INDEX_COMMAND_NAME = "Git add to index";
Copy link
Contributor

Choose a reason for hiding this comment

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

You have two equals constants in action and presenter, consider to extract it to the shared place of interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to constants

consolesPanelPresenter.addCommandOutput(appContext.getDevMachine().getId(), console);
notificationManager.notify(constant.statusFailed(), FAIL, FLOAT_MODE);
public void showDialog() {
if (appContext.getResources().length == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

appContext.getResources() may be null add additional checking for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* @param all
* automatically stage files that have been modified and deleted
* @param files
* the list of iles that are commited, ignoring the index
Copy link
Contributor

Choose a reason for hiding this comment

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

iles -> files and commited -> committed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

HTML message;
@UiField
TextArea items;
Label message;
Copy link
Contributor

Choose a reason for hiding this comment

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

Reformat this code.

this.items.setText(sb.toString());
}
public void setMessage(String message) {
this.message.getElement().setInnerHTML(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.message.setText(message);.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message parameter is given in html format, and setText() would display the raw massage with html tags. How about just to rename this parameter to htmlMessage?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it would be better to rename the parameter.

* @param includeSelection is selection included
*/
void setIncludeSelection(boolean includeSelection);
/** Returns true if the project explorer selection must be added to index before commit, and <code>false</code> otherwise. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Simple selection, it might not be from project explorer. Paraphrase this sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

String selectedItemPath = selectedItem.getLocation()
.removeFirstSegments(appContext.getRootProject().getLocation().segmentCount())
.toString();
for (IndexFile file : indexedFiles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap this block of code in curly brackets according to our coding style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

//Mark selected items to reset from index
for (Resource selectedItem : appContext.getResources()) {
String selectedItemPath = selectedItem.getLocation()
.removeFirstSegments(appContext.getRootProject().getLocation().segmentCount())
Copy link
Contributor

Choose a reason for hiding this comment

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

The same as above, you can simply remove the first segment from the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@codenvy-ci
Copy link

Build # 1525 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1525/ to view the results.

@codenvy-ci
Copy link

Build # 1532 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1532/ to view the results.

@codenvy-ci
Copy link

Build # 1534 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1534/ to view the results.

* @param amend
* indicates that previous commit must be overwritten
*/
Promise<Revision> commit(DevMachine devMachine, Path project, String message, boolean all, Path[] files, boolean amend);
Copy link
Contributor

@vparfonov vparfonov Jan 10, 2017

Choose a reason for hiding this comment

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

why we need Path[] files if you added all options ? If all will be true what need to set to the files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all parameter means add to index all changes and files parameter defines what files to commit. all can be true and files parameter can contain items, this will happen when user would call commit operation on selected items and would select Add all changes except of new files check box.
We have separate issue: #3614 to improve this window where add to index action will be performed automaticaly because user will have an ability to chose all changed files directly in the window.

@vinokurig vinokurig merged commit a899f50 into master Jan 10, 2017
@vinokurig vinokurig deleted the CHE-3271 branch January 10, 2017 15:25
@codenvy-ci
Copy link

@slemeur slemeur added the kind/enhancement A feature request - must adhere to the feature request template. label Jan 18, 2017
@slemeur slemeur added this to the 5.1.0 milestone Jan 18, 2017
@JamesDrummond JamesDrummond mentioned this pull request Jan 31, 2017
9 tasks
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants