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

Issue737 treebrowser follow doc fix #796

Closed
wants to merge 5 commits into from

Conversation

eht16
Copy link
Member

@eht16 eht16 commented Dec 2, 2018

This fixes following the current document path.
As mentioned in #737 this was broken on Windows but actually it was broken on non-Windows as well if the start directory (Geany's default open directory or the project base directory) has no common parts with the current document's file path.

For Windows, there was an additional hack necessary: ignoring the drive letter completely in all checks.
This made it working for the moment but obviously breaks a lot of the plugin's functionality if files on another drive than the current are handled.
This should be fixed properly but since the fact that I don't use the plugin and my available time is limited, I won't do myself.

… file in tree

If the current file path is not part of the current directory,
the code starts at / to find the path. However, it always skipped the
first path part (i.e. /) and so never matched.
This happened most often when Geany's default open directory has
no common parts with the file path of the current document (e.g.
Geany's default open directory is /tmp and the file path is like
/home/user/src/somefile.c).
This is rather a hack than a fix: we ignore the drive letter completely
on Windows and use \some\path consequently in the code.
This breaks multi drive support for now but fixes many path related
features like 'Follow current document' which was also broken before.
@eht16
Copy link
Member Author

eht16 commented Dec 3, 2018

Ignore 428b6d5, I will replace this commit with a better solution as stripping the drive letter breaks more than it solves.

@frlan
Copy link
Member

frlan commented Dec 16, 2018

@eht16 I assume you like to rebase this before merging?

@eht16
Copy link
Member Author

eht16 commented Dec 16, 2018

Sure but right now I don't want this merged at all (see my comment above as well as the WIP label).
This needs more work.

@frlan
Copy link
Member

frlan commented Dec 17, 2018

@eht16 OK. Reopen PR once done.

@frlan frlan closed this Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants