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

[RFC] Aucmds refactor and Downloadpost #1495

Closed
wants to merge 4 commits into from

Conversation

antonva
Copy link
Member

@antonva antonva commented Apr 23, 2019

This addresses issues #810 and #807. An optional argument of the type
AutocmdArgs can be passed to loadaucmds. Only a small subset of the variables
mentioned in #810 are implemented. I think it's better to implement these as the
need/request arrives but for the DownloadPost aucmd I felt like this was the
most straightforward way to implement.

The parseaucmdargs function takes an aucmd and looks for the pattern "<param">
where param is any of the keys provided by AutocmdArgs and replaces the pattern
with the matching information from the args.

Example usage:
:au DownloadPost .* fillcmdline "<file>"
Will print the full path of the downloaded file on the cmdline.

I'm not attached to the naming convention but it's the same as pentadactyl used.

I have yet to write the documentation for the feature.

Any comments welcome.

I've done away with the string literal type signature in loadaucmds and replaced
it with an internal type of AUTOCMDS, this means that the AUTOCMDS object
(formerly an array) is now the de facto truth of which autocmds exist in the
extension.
This addresses issues tridactyl#810 and tridactyl#807. An optional argument of the type
AutocmdArgs can be passed to loadaucmds. Only a small subset of the variables
mentioned in tridactyl#810 are implemented. I think it's better to implement these as the
need/request arrives but for the DownloadPost aucmd I felt like this was the
most straightforward way to implement.

The parseaucmdargs function takes an aucmd and looks for the pattern "<param">
where param is any of the keys provided by AutocmdArgs and replaces the pattern
with the matching information from the args.

Example usage:
`:au DownloadPost .* fillcmdline "<file>"`
Will print the full path of the downloaded file on the cmdline.

//#content_helper
export function parseaucmdargs(aucmd: string, args: AutocmdArgs): string {
const re = /^\"<(\w+)>\"$/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How hard would it be to factor this out to into a well-contained library so you can add some simple test cases? It looks right, and I expect that Pentadactyl collected enough real-world observations to establish that strings like <file> and such didn't occur that often in URLs, but I'd prefer to not start accidentally replacing random other <....> pairs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for factoring it out into a library, the problem there would be the implementation. If we remove loadaucmds from excmds along with the parser I'm guessing it'd have to live in the content script somehow similar to what autocontainers does in the background.

The current implementation is pretty heavy handed so I'm nigh sure you won't get it mixed with urls or other things. I split the aucmd string on spaces and check for exact matches on ^"<WORD>"$. The problem with this is that you can't combine params, "<file> <url>" will not be matched since the split on space would create "<file> and <url>" which won't match the regex.

@bovine3dom
Copy link
Member

@antonva any thoughts on #1885? They look fairly tangential, except WebRequest has its own filter syntax, so we might need to write some conversion between the two.

@antonva
Copy link
Member Author

antonva commented Oct 8, 2019

@bovine3dom it doesn't behave all that different either if I remember correctly.

I must admit I had to read over what I had originally proposed here as it's been a while, but something very similar could be set up for webrequests.

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.

3 participants