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

Support relative project paths (by providing absolute path property) #600

Closed
wants to merge 2 commits into from

Conversation

deeptoaster
Copy link

This is a different approach to #586 as suggested in #32 (comment).

According to the documentation, the relative path ./ can be used to refer to the directory of the project file.

However, this is not the observed behavior in the app. If such a relative path is set in the project properties, the Open File, Save File, Find in Files, and other dialogs default to ~ instead of the actual project directory. In addition, the project directory is not added automatically to the Open File and Save File dialog sidebars, as they are added when using an absolute path for the project directory.

This pull request fixes this bug in the three dialogs mentioned and possibly in other places.

@elextr
Copy link
Member

elextr commented Aug 6, 2015

There seems to be unrelated changes in the commits, please check and correct.

@deeptoaster
Copy link
Author

My apologies—I've rebased it to remove the unrelated commits.

@codebrainz
Copy link
Member

Couldn't this just use project_get_base_path() everywhere instead of adding a new abs_path field? Seems like it might be easier and prevent some of the dangling pointers. The only weird thing is having these paths in UTF-8 encoding rather than locale encoding. Probably base_path is stored in UTF-8 because it's shown in the GUI, but otherwise, it seems like a bad idea to keep it in UTF-8.

If base_path was kept in locale encoding (and converted to/from UTF-8 when presenting to user interface), then abs_path could simply be replaced with tm_get_real_path(project->base_path) and avoid issues with dangling pointers and adding extra struct fields.

@deeptoaster
Copy link
Author

I got the idea to use a new field from #32 (comment); it seemed cleaner to not have a newly allocated path string every time the project path is used, as using project_get_base_path (and tm_get_real_path(project->base_path)) would entail.

If it would be preferable to use the alternative approach, I can put it in another merge request, but that would mean additional code to free each bsae path string created, as #586 (comment) pointed out.

I didn't notice the dangling pointers, thanks. I'll put some fixes in when I get home.

@codebrainz
Copy link
Member

I doubt the allocations matter much, as long as they're are properly freed. There might be some IO overhead in tm_get_real_path() though, via realpath(), although for this use it might not matter either.

To be honest I just did a quick skim of the diff on my lunch break and didn't review the implications of the PR overall, I just made a couple comments when I saw the potential UB/encoding issues.

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