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

Try harder to get the git root directory the first time GitGotoDiff is called #524

Merged

Conversation

vanrijn
Copy link
Contributor

@vanrijn vanrijn commented Jun 22, 2017

The problem is that we always prompt the user for the root git directory. GitWindowCommand has a nice little addition to its get_working_dir() method to try to get an open folder from the window as a last ditch effort. This change refactors the code from GitWindowCommand.get_working_dir() out into a reusable method named get_open_folder_from_window(). GitGotoDiff can now use the new get_open_folder_from_window() method to try to get a git root directory if one hasn't been set yet.

… git root directory first time GitGotoDiff is called.
@vanrijn
Copy link
Contributor Author

vanrijn commented Jun 22, 2017

I believe this might also help to address issue #491 albeit in a slightly different way than pull request #496

@watergear
Copy link

Thank you for your work. But the #496 can not be solved by this change. I pull your commit and test failed.

As my said at #496, active view is changed and the new view window can not get the file path. That means it is too late to get the git root path. At the moment, self.active_file_path() and window.folders()[0] are both None. It is no way to get git root path back again.

The solution is to get the git root path before creating new scratch view, and then store the git root path into new view.

@vanrijn
Copy link
Contributor Author

vanrijn commented Jul 1, 2017

@kemayo This fixes an annoyance in that we currently ask the user for the git root every single time. I've been using this in my own work for the last week and it's made a huge difference in the quality of this plugin for me. Should I file an issue for this to help get it merged in? Or is there anything I can do better to get it merged in? Thanks!! =:)

@vanrijn
Copy link
Contributor Author

vanrijn commented Aug 25, 2017

Meh, sorry to bug you again, but I just hit this on my second machine where I'm not running with my patch here and this is really annoying. This patch makes the experience so much better. Any thoughts, @kemayo?

@kemayo kemayo merged commit a0ee9b5 into kemayo:master Aug 25, 2017
@vanrijn
Copy link
Contributor Author

vanrijn commented Aug 25, 2017

Thank youuuuu!! <3

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.

None yet

3 participants