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

Project Management #2308

Merged
merged 82 commits into from Oct 28, 2019
Merged

Project Management #2308

merged 82 commits into from Oct 28, 2019

Conversation

@wadethestealth
Copy link
Contributor

wadethestealth commented Oct 16, 2019

Description of the Change

This is a functional realization of the project selection prototype in #1934. For those unfamiliar with the PR, this would allow you to view the base name of your currently active project folder, as well as actively change your active project folder within the Git and GitHub panels.

Note: This is a minimum viable product of a small portion of #1934, and the rest is meant to implemented in separate PR's, which will be updated here as they arrive.

Functional Realization of Avatar PR: #2325, #2348
Functional Realization of PR Refresh Button PR: N/A

Achieved:

  • Add Tab Header View to Git Panel Tab
  • Add Tab Header View to GitHub Panel Tab
  • Integrate new project context priority queue
  • Allow users to switch active projects in the Tab Header View
  • See tests for test coverage achievements.

Screenshot/Gif

project-management

Alternate Designs

N/A

Benefits

  • Support for switching current active projects
  • Better support for multi-project workflows

Possible Drawbacks

  • Loss of Vertical UI Space

Applicable Issues

#1934

Tests

  • Added Tab Header View Unit Tests (100% coverage)
  • Added Tab Header View Shallow Tests To GithubTabView and GitTabView (100% coverage)

Documentation

TBD

Release Notes

TBD

User Experience Research (Optional)

I welcome all to critique.

Copy link
Contributor Author

wadethestealth left a comment

const handleActivePaneChange = item => {
const activeRepository = this.getActiveRepository();
const activeRepositoryPath = activeRepository ? activeRepository.getWorkingDirectoryPath() : null;
this.scheduleActiveContextUpdate({activeRepositoryPath}, {item});
};
const handleProjectPathsChange = projectPaths => {
const activeRepository = this.getActiveRepository();
const activeRepositoryPath = activeRepository ? activeRepository.getWorkingDirectoryPath() : null;
this.scheduleActiveContextUpdate({activeRepositoryPath}, {projectPaths});
};

These new functions allow for better contextual updates, by passing their value to the extras argument.
This allows us to better determine did a project path update our context, did an item pane update our context, or was it something entirely different.

Copy link
Contributor Author

wadethestealth left a comment

this.contextPool.set(workdirs, savedState);
if (savedState.activeRepositoryPath) {
// Preferred git directory (the preferred directory or the last serialized directory).
return this.contextPool.getContext(savedState.activeRepositoryPath);
}
const active = await fromPaneItem(this.workspace.getCenter().getActivePaneItem());
if (active.itemPath) {
// Prefer an active item
return this.contextPool.getContext(active.itemWorkdir || active.itemPath);
}
const projectPaths = this.project.getPaths();
if (projectPaths.length === 1) {
// Single project
const projectPath = projectPaths[0];
const activeWorkingDir = await this.workdirCache.find(projectPath);
return this.contextPool.getContext(activeWorkingDir || projectPath);
}
if (projectPaths.length === 0 && !this.activeContext.getRepository().isUndetermined()) {
// No projects. Revert to the absent context unless we've guessed that more projects are on the way.
return WorkdirContext.absent({pipelineManager: this.pipelineManager});
}
return this.activeContext;

This is by far the biggest change to the PR is using the last saved active path, which enables a user to choose rather than having the active pane choose. IMO, active pane repository detection could be removed, and we could always just select the first project path, which could simplify the code, because the active pane detection branch now only runs for small edge cases, where benefit is negligible. However, I have also considered allowing a toggle for people to prioritize active pane or have them choose. I would love to here thoughts about this. I also left it here as a regression technique for now in case I have a logic error, that would provide an absent context.

@wadethestealth wadethestealth force-pushed the wadethestealth:project-management branch from c41c1c2 to 989e727 Oct 16, 2019
@wadethestealth wadethestealth force-pushed the wadethestealth:project-management branch from 989e727 to 61a39e3 Oct 16, 2019
@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Oct 16, 2019

IMO, active pane repository detection could be removed, and we could always just select the first project path, which could simplify the code, because the active pane detection branch now only runs for small edge cases, where benefit is negligible.

👍 Yeah, I'd much rather get rid of this complexity entirely. Explicit choice all the way.

However, I have also considered allowing a toggle for people to prioritize active pane or have them choose. I would love to here thoughts about this.

I think it'd be better to fully commit to the explicit choice from the context tile. It's a pretty significant simplification of behavior and less surprising all around. And it'd be nice not to be stuck maintaining both.

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Oct 16, 2019

@wadethestealth Is this PR intended to replace #2299, or are they independent?

@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Oct 16, 2019

@smashwilson they are independent, but I unfortunately have no where to put remote selection again. I can update it with new ui, but I will need a place to put it. I know @simurai suggested status bar in a separate issue a long while ago, but it was not swung one way.

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Oct 16, 2019

Hmm. Maybe we can unify them into the same dropdown, one entry per project root - remote pair? Like this:

prototype

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Oct 16, 2019

Oh, also, you can steal markup and CSS from the prototype branch:

https://github.com/atom/github/compare/prototypes/fr1934-context

@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Oct 16, 2019

@smashwilson, I don't hate it but that is a lot of tiles already. It's definitely functional. And yes I have been doing so (first commit on this branch is authored by simurai from that branch)

Copy link
Member

smashwilson left a comment

Awesome! Nicely done. Let's 🚀

@smashwilson smashwilson merged commit aae4848 into atom:master Oct 28, 2019
6 checks passed
6 checks passed
atom.github Build #20191026.5 succeeded
Details
atom.github (Lint) Lint succeeded
Details
atom.github (Linux) Linux succeeded
Details
atom.github (MacOS) MacOS succeeded
Details
atom.github (Snapshot) Snapshot succeeded
Details
atom.github (Windows) Windows succeeded
Details
@wadethestealth wadethestealth deleted the wadethestealth:project-management branch Oct 29, 2019
@wadethestealth wadethestealth mentioned this pull request Oct 29, 2019
8 of 8 tasks complete
@lkashef lkashef mentioned this pull request Nov 6, 2019
@aviatesk

This comment has been minimized.

Copy link

aviatesk commented Nov 7, 2019

@wadethestealth @smashwilson
So after this PR, github package doesn't automatically switch its active repository according to the current active editor, right ?
(In other word, now this package is kinda sticky to the selected repository)

I found the previous behaviour is more useful, especially when editing files outside of the current projects.
Any discussion on this ?

@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Nov 7, 2019

@aviatesk There had been talk of this feature a while ago, and during their research I believe they decided it was an unnatural experience and was confusing to users. I merely implemented, what was already decided/designed a while back. There is a bit of discussion on one of my resolved comments, where I thought about leaving in the active pane switching and having a setting, but it was decided, that it was better to simplify code by removing it. It inherently slows down cross project workflows, but allows for clarification of what you are in. Also note that the active pane was not always the active context, and it was also a guess as to what a user wanted, whereas this is explicit and user determined.

@aviatesk

This comment has been minimized.

Copy link

aviatesk commented Nov 7, 2019

@wadethestealth Thanks for the reply.
Still I personally object to the idea, but could you tell me if there is any command to switch the active repository ? With the current behaviour I want a way to switch it without any GUI but with a shortcut, at least.

@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Nov 7, 2019

@aviatesk there is none right now, but that would definitely be a good add. I wonder what this would look like though. Would it just give focus to the expanded select and you use arrow keys like normal? I think that's probably the best option.

@aviatesk

This comment has been minimized.

Copy link

aviatesk commented Nov 7, 2019

That could definitely be one option, and I guess creating a separate selector view (like what can be provided by https://github.com/atom/atom-select-list) could be an alternative.

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Nov 7, 2019

@aviatesk: Yeah, you can see the writeup of the motivation and vision for context management on the feature request PR: #1934 (note the "rendered" link in the PR description).

I'm a definite 👍 on having at least one mouse-free way to change the active git workdir context. Another relatively quick-to-implement idea might be to add a command-palette command that jumps to the context of the current active pane item?

Can you open a new issue to discuss this? Make it a little easier to discover than comments on a merged PR 😄

@aviatesk

This comment has been minimized.

Copy link

aviatesk commented Nov 8, 2019

@wadethestealth Thanks for opening the issue :) Just for a reference, anyone interested in this can find the issue here: #2335

@wadethestealth wadethestealth mentioned this pull request Nov 21, 2019
2 of 2 tasks complete
aviatesk added a commit to aviatesk/github that referenced this pull request Nov 28, 2019
…gement"

This reverts commit aae4848, reversing
changes made to 79afbab.
@mordash

This comment has been minimized.

Copy link

mordash commented Jan 16, 2020

Hi,

is there an option to disable this list ?
It really is not cool when working on several projects at the same time.

Best regards.

@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Jan 16, 2020

@mordash currently no, but there is a pr not merged that adds a shortcut to switch to the git project of the current file you are in, it's this shortcut is supposed to both facilitate old behavior and make switching easier. We know it is cumbersome for multiproject workflows, but the change was necessary to remove confusion on how project selection worked.

@mattlyons0

This comment has been minimized.

Copy link

mattlyons0 commented Jan 19, 2020

I do not believe any of the options listed in #2335 are sufficient to make the new behavior as good as the exiting behavior was.

If you are switching projects quickly finding the currently selected project from a dropdown can be difficult, especially with many projects open. Adding a shortcut to open the currently selected project is a nice step in the right direction but it is still an extra step from before.

I have added a setting to restore the old behavior of selecting the project based on the active panel in #2393 . I like this option because it gets around the confusing aspect since it is an opt-in experience and disables the dropdown but keeps it visible showing the current project name.

That said it is possible in addition to this solution adding a command to select the current project still has value for users who do not want to go all the way of enabling this new setting.

@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Jan 19, 2020

@mattlyons0 I'm sorry you feel this way. The main reason for the change was because the old way was extremely user unfriendly, and instead of adding an option to maintain the old version and the new version, we opted out of keeping the old version in to reduce technical debt and code complexity as the old decision algorithm used guessing what the user wanted rather than explicit choice. This is not to deny your or, but rather to explain the context around the situation and some of the relevant to your case avenues of the many avenues discussed. Thank you for the feedback on this change as well.

@mattlyons0

This comment has been minimized.

Copy link

mattlyons0 commented Jan 19, 2020

@wadethestealth The old behavior can be conditionally added with 19 lines of code in a single file as I have done so in this commit: 08aef18

I do not see that as a large amount of technical complexity to maintain a feature that is notably useful for users working in multiple repositories. This has only been released for 4 days (in Atom Stable 1.43) and already 7 users have found this several month old pull request indicating they would prefer the old behavior.

I don't think the old behavior is for everyone but it certainly has value for power users, of which I think Atom definitely has a lot of (given a major difference between Atom and VSCode is its flexibility). Putting this behind a configuration setting at the cost of merely 2 functions of additional technical complexity added seems like a valuable addition to me.

@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Jan 19, 2020

@mattlyons0 it has only officially be released this long, but it has be in the nightly for a while. Also there will always be back lash to change in any degree. There are also a lot more atom users than 7 and of course people frustrated with the change are a lot more likely to seek out this pr and comment. That being said you provide fair points. I am not a maintainer of this repo and this change is a very old todo from a group of the previous maintainers. Ultimately the decision comes down to @smashwilson or an official atom team member.

@jarvelov

This comment has been minimized.

Copy link

jarvelov commented Jan 20, 2020

Another user in favor of the old autoselect behavior here. I thought this was a bug introduced in 1.43. I don't see why we can't have both the git repo select and the old autoselect functionality. If this functionality can be implemented in the 19 lines of code @mattlyons0 I don't see how that code is complex and the feature is very valuable to me.

I, not being hyperbolic, always work with multiple repos and have to switch between them a lot and now to have to manually change the git repo adds another step for me that I previously didn't have to do.

I'm fine with it not being the default behavior but please add back the old functionality as a configuration setting.

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Jan 20, 2020

👋 Hey everyone. Can we centralize discussions about this on #2335 instead of here? Comments on merged PRs are hard to discover for non-involved users and don't have a good way to signal when we reach some kind of consensus.

I'll 🔒 this so this comment with the issue link is the last one and everyone can see it 😉

@atom atom locked and limited conversation to collaborators Jan 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.