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

Add $editorMapping support #170

Closed
wants to merge 1 commit into from
Closed

Add $editorMapping support #170

wants to merge 1 commit into from

Conversation

@adrianbj
Copy link
Contributor

adrianbj commented May 15, 2016

@rostenkowski

This comment has been minimized.

Copy link

rostenkowski commented May 16, 2016

This feature would be nice for anybody using some remote/virtual/production stack.

If I am reading that build information right there is only used the array(...) syntax instead of [...].

@dg

This comment has been minimized.

Copy link
Member

dg commented May 16, 2016

@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

@dg - I am sorry if I am missing something, but doesn't that open-editor.js file only get used on Windows systems?

@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

As a follow up, the other problem with the open-editor.js approach is that it I assume it affects all links to editor:// not just those coming from Tracy. This may not always be desired.

@dg - Just wondering if you completely opposed to implementing something along the lines of this PR? I would just like to have an idea as to whether I need to continue to hack the Tracy core to make it functional for my needs. Perhaps you would consider some other approach that would allow editor filepath mapping that works on all systems, is just for Tracy links, and is easily configurable by the user or the dev packaging Tracy for specific purposes

Thanks for reconsidering.

@dg

This comment has been minimized.

Copy link
Member

dg commented May 16, 2016

You are right that open-editor.js is platform specific, I'll add it.

@dg dg closed this in 11eab77 May 16, 2016
@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

Thanks @dg - the problem is that it doesn't work for me. The substr / strlen combination only works if the replacement is right at the beginning of the path.

Is there any reason you didn't like the strtr approach which allows for much more flexibility and no need for loops? I actually thought that strtr was generally faster anyway.

@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

Oh and one more comment - your approach only changes the link, but not the displayed URL. Perhaps that is desirable in some situations, but in my case I'd also prefer it to be changed as well, which is why I also has the strtr line in the editorLink method.

Perhaps this could be a configurable option.

@dg

This comment has been minimized.

Copy link
Member

dg commented May 16, 2016

I supposed that you want to change begin of path - or not? Can you describe you use case?

@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

I guess that might be the case for some users. In my situation I need to convert from a path where there are compiled files to the path of the original files. Files are compiled at runtime if there have been any changes made.

For example, I need to convert:

/Users/ajones/Sites/pwstable/site/assets/cache/FileCompiler/site/templates/home.php

to

/Users/ajones/Sites/pwstable/site/templates/home.php

So my $editorMapping looks like this:

Debugger::$editorMapping = array('/site/assets/cache/FileCompiler' => '');

which works perfectly with my original strtr approach.

@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

Just to clarify - I guess I could get the full path and replace that with the fullpath without the /site/assets/cache/FileCompiler but my approach still seems more flexible unless you have a good reason for not wanting to allow that.

I would also like your thoughts on the ability to modify the displayed URL as well as the actual link. I was thinking maybe a new option:

$editorMappingReplaceDisplayedUri = true;

It's a very long and ugly variable, but maybe that's ok?

dg added a commit that referenced this pull request May 16, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented May 16, 2016

Ok, repushed.

@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

Thanks - that works much better I think.

Quick question - I see you moved the strtr in the editorLink() method so that it's inside the "if". Does it make sense that we wouldn't also want to translate the $file in the "else"

return self::formatHtml('<span>%</span>', $file . ($line ? ":$line" : ''));

I don't really have any experience calling methods from the OutputDebugger class (which I think is the only situation when that conditional would default to "else"), so maybe it wouldn't be appropriate, but thought I should ask.

Sorry for taking up so much of your time on this PR.

@dg

This comment has been minimized.

Copy link
Member

dg commented May 16, 2016

It is inside because 1) is_file($file) must be called with original file name and 2) we don't want to call strtr two times

@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

Right - makes sense to prevent the calling of strtr twice.

I still don't know about:

return self::formatHtml('<span>%</span>', $file . ($line ? ":$line" : ''));

If this is triggered it will return the $file with the original path. I haven't come across this scenario, but I'll keep an eye out for it.

dg added a commit that referenced this pull request May 16, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented May 16, 2016

Oh, now I see! Fixed :-)

@adrianbj

This comment has been minimized.

Copy link
Contributor Author

adrianbj commented May 16, 2016

Perfect, I think that should cover everything now - thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.