Remove userInclude/userExclude from Config.parse() #1467

Closed
arantius opened this Issue Nov 11, 2011 · 6 comments

4 participants

@arantius
Collaborator

Still have to verify that it's the right thing to do, but I just read Config.parse() and was confused/worried about the userInclude/userExclude cases there. That's not supported metadata is it?

https://github.com/greasemonkey/greasemonkey/blob/968c1808344285e7aa301aa1ed5aba4b48899cbc/content/config.js#L185

Note that this is not the config.xml version that should be kept:
https://github.com/greasemonkey/greasemonkey/blob/e9c72f69362604ab35b50fb46011e5be2880722e/content/script.js#L281

@johan
Collaborator

None that we have had any conclusive GM-dev discussion on, at least, but it looks like an implementation detail of how writing back the includes and includes edited in the manager to the script works after a user edits them, judging by #1362 commentary. It stems from your commit here, if that is any help.

You didn't conclude what your thinking or intent was around that when the matter was discussed on GM-dev in the UI for editing @include and @exclude rules and later the (hopefully) final 'clude editor thread threads (there were others in-between, too, but IIRC those two were the good ones).

On the surface, it kind of looks like you borrowed ideas from kwah's post near the end of the latter. By the end of that thread I felt I had been doing too much discussion and too little coding around there and phased out for a bit, but I'm just as curious as you about what to make of this thing.

@Martii

What is your reasoning behind removal? Seems like a really neat feature that helps end users be able to easily add/remove predefined injection points without editing the script.

Currently leaning toward -1 but I am open.


EDIT: Here is a proof of concept user.js. A lot of the users remove sites by editing... this would allow the UI to be used instead. What does everyone think?

@johan
Collaborator

If this ticket's intent was to revert #1362 and remove the feature "user editable includes", I think it would say so more clearly.

As I read it, it's just on removing an unintentional implementation effect: leaking a redundant header into scripts in a way not intended. Or at least back-pedaling a little to figure out what the specs should be, and then properly enforcing them in code.

@arantius
Collaborator

For a period of time, "@user" headers were discussed. Implementation is impractical, and altering the files is unsafe. So we didn't actually do that (these have no effect and are never read, unless I'm seriously confused). This feature (#1362) exists, and I do not propose removing it. Just dead code.

@Martii

Well I didn't follow the UI discussions in great detail but here on this network the POC referenced above does populate the User Settings UI in GM and does inject (or not if userExclude). Seems like a tidy way to keep things distinct and clear for ScriptWrights without having them select all the Script Settings and move them over one at a time. So I'm not sure what you mean by "dead code".

@sizzlemctwizzle

does populate the User Settings UI in GM and does inject (or not if userExclude).

Guess that means the code isn't completely dead.

Implementation is impractical, and altering the files is unsafe.

Yeah, I thought that was the conclusion we came to on the thread. I'm +1 for removing these keys (and the associated code that uses them) because I think they'll just lead to confusion among users (e.g. if a user removes one of their includes and the script writer adds a @userInclude in the next version does the user need to remove that via the UI? What happens if they remove them from the script? Does that remove them from the UI as well?) I'd like to keep user 'cludes and script 'cludes completely separate for clarity. One is in the script and one is in the UI.

@arantius arantius added a commit to arantius/greasemonkey that referenced this issue Dec 8, 2011
@arantius arantius Completely refactor Config.parse() into a standalone module.
Roundabout way of fixing #1467
2366075
@arantius arantius closed this Dec 8, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment