Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@wadethestealth
Copy link
Contributor

@wadethestealth wadethestealth commented Nov 18, 2019

Description of the Change

  • Adds github:select-active-pane-directory command to RootController

This allows a shortcut for switching between projects directories without the need to interact with the UI.

Alternate Designs

Please read #2335.

Name of command is up to discussion and probably needs tweaking.

Possible Drawbacks

Re-introduced active pane complexities, but they are much simpler this time around and are user initiated.

Applicable Issues

Fixes #2335

Tests

  • Adds tests to command in RootController

@aviatesk
Copy link

AFAIU, this only allows us to switch between repositories in current project, right ?
(because changeWorkingDirectory internally calls getNextContext and now it only works for repositories under the current project)

How about letting an user to switch to a repository outside of a current project ?
As far as s/he switches to the repository on purpose, there won't be much confusion, and it would help lots of cases e.g. when we want to just check/modify the repository state that is not in a current project, i.e. dot files or something like that)

@wadethestealth
Copy link
Contributor Author

wadethestealth commented Nov 23, 2019

@aviatesk Yes this is true it does only work with current projects. This is just old code glued together in a different location, but the code used to be inside getNextContext and it would automatically assign the found context now it doesn't. These are some things that I am thinking about, but they may already have answers I just need to dig. What happens when you change from an active context that is outside the project to a new context? Will the context ever be cleaned up? I don't remember if active panes used to be considered workdirs, but if they were I could re-add that functionality. I am just trying to not stray into the territory of new functionality just adding a shortcut to facilitate old. If it does require new functionality, I will need to do some looking into.

@aviatesk
Copy link

Will the context ever be cleaned up?

Does it really need to be cleaned up ? I guess this package caches the found repository contexts (maybe in contextPool i guess ?), so they would be cleaned up when this package is deactivated anyway, wouldn't they ?

In other word, I can't see the reason why only the repository outside of a current project needs to be cleaned.

@wadethestealth
Copy link
Contributor Author

The context pool is only meant to hold active working directories, which should be cleaned up when you no longer need that directory because there are filesystem observers running all the time it is in the contextpool. The cache is meant to easily refind repositories but we don't want observers to keep running because if someone accesses a lot of different outside project files we could cause a large slowdown

@aviatesk
Copy link

aviatesk commented Nov 25, 2019

But at least we held those repositories outside of a current project in that within the previous implementation, right ?
So, I'm really interested in the benchmarks that you base on.

This is just my guess, but maybe within the previous implmentation, the real performance problem that an user has faced would be caused by that the context switches everytime on active item observation, and not by those subscriptions to changes in a repository.

@wadethestealth
Copy link
Contributor Author

Again I haven't investigated it yet. It was just thoughts. Once I get around to it, I bet there will be a clear answer.

@wadethestealth
Copy link
Contributor Author

@aviatesk So previously, the contexts outside of current project were added while the file was in an open pane and being focus. However after you focused on another pane, this context would be removed. Essentially the pane was entirely dynamic, and it isn't hard to implement this. However, there is a potential draw back since we don't factor in active panes by default. Assuming I add in the override for an active pane to be dynamic like before, you would switch to the active pane and the result would appear in the drop down project selection. After you change projects though, the out of project file working directory will disappear even if the pane is still open and being focused on. This can easily cause user confusion. There are a couple of methods I could try to implement to work around this.

  1. We account for all open panes in the center of the workspace by default (this only differs from the old version by tracking all of them rather than the focused one)

  2. I implement the use of state for scheduling context by adding a set of workdirs to consider to add during getNextContext. This state will be added to when you use the switch command on a file out of project, and it would be removed from when the file's pane is no longer in the workspace center (closed or moved to a different dock). This could lead to subtle ux confusion because some people will never find this command and therefore not have access to git repositories outside of projects and others may a ui way to use the feature.

@smashwilson was not very fond of the active pane decision happening by default in the previous implementation because of code complexity and ux reasons, but since ux reasons have since been solved, it may be ok to reintroduce a cleaner version of the code complexity.

Opinions? @aviatesk @smashwilson

@edahlseng
Copy link

edahlseng commented Jan 16, 2020

I'm just finding my way here after a recent Atom upgrade changed the way the GitHub pane works. The updated project selector is super awesome, but makes it much harder when working across multiple projects. What's needed to get this PR pushed through?

@wadethestealth
Copy link
Contributor Author

@edahlseng tests as I couldn't figure it out, but also the current state of this implementation does not work on files outside of the projects context as doing how I stated above (dynamically removing context when file is closed) is extremely computationally expensive. I'm not so worried about the out of project file, but there should definitely be tests for the current state. After that, @smashwilson or another atom member familiar with atom GitHub will have to give the pr an approval and merge.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Git context management

3 participants