Move URL to yankable string conversion to urlutils

[michaelfm1211]

Closes #7693

As per TheCompilers' suggestion, this PR creates a get_url_yank_text function in qutebrowser.utils.urlutils that takes a QUrl (and a parameter indicating if the FormatOption.DECODE_RESERVED flag should be used in place of FormatOption.ENCODED) and returns it as a string that is suitable for yanking (ie: removes passwords, ignored parameters).

[michaelfm1211]

@The-Compiler I just checked back on this and I don't know why those two tests are failing; they don't look like they're related to this PR. If they're unrelated, then is it possible to skip them somehow?

[The-Compiler]

You can ignore them for now. It looks like something broke with a Qt upgrade in an experimental environment. I'll need to take a closer look, but haven't gotten around to it yet.

[toofar]

Looks pretty good after an initial look. Nice isolation of the new function managed when pulling it out, changing the what arg to a boolean seems reasonable (if only to avoid having to add a new enum).

Could you have a look at changing the description of the url.yank_ignored_parameters setting in qutebrowser/config/configdata.yml to be more generic?

Speaking of more generic, I think there is a couple of other places this new method could be used if you are up to it:

• qutebrowser/mainwindow/prompt.py::prompt_yank
• the {url} substitution in yank inline (try the default binding ym)

Lastly, do you have any thoughts on the test cases in tests/end2end/features/yankpaste.feature? Are there any that are now redundant?

Proposed changelog entry: Ignored url query parameters (via url.yank_ignored_parameters) are now respected when yanking URL via hints too (for example, via hint links yank) (#7879)

[michaelfm1211]

@toofar Thanks for the feedback! I'm pretty new to the codebase so those file paths and usages of yanking were useful.

I just added another commit that uses the new function in qutebrowser/mainwindow/prompt.py::prompt_yank, which was pretty straightforward.

Currently ym is an alias for yank inline {url}, and I'm not sure if {url} should strip ignored arguments because that might be a bit counterintuitive and break something. I think the best way to have ym respect the ignored parameters is to create a new {yank_url} substitution that works the same as {url} but uses the new function instead, then change ym and yM to use {yank_url} instead of {url}. This is what I did in my commit. Is this a good way to approach it?

I also added some new ones to cover the edge cases you proposed. Looking through tests/end2end/features/yankpaste.feature, it looks like the units cover "Yanking URLs with ref and UTM parameters" and "Yanking URLs with ref and UTM parameters and some other parameters", but I haven't made any changes to them just in my last commit.

[toofar]

Currently ym is an alias for yank inline {url}, and I'm not sure if {url} should strip ignored arguments because that might be a bit counterintuitive and break something. I think the best way to have ym respect the ignored parameters is to create a new {yank_url} substitution that works the same as {url} but uses the new function instead, then change ym and yM to use {yank_url} instead of {url}.

Hmm, I was thinking to just modify the {url} substitution itself. The docs for {url} (which is a substitution for all command positional args) says:

{url} expands to the URL of the current page

And the config param in question is url.yank_ignored_parameters. So while I think that if you set ignored params you probably want them ignored everywhere I do see your point that the behaviour change might be surprising. So I suppose I'm fine with introducing a new param instead. Although another option is adding a {url:raw} for the (presumed) edge case when people want to use the URL with "ignored" params still in it.

{yank_url}

The other substitutions don't have underscores in them. There is already {url:pretty} that just modifies the formating of the whole URL. What do you think about making this one {url:yank} instead of having the underscore? (I'm not sure about the "yank" term, since it can be used with anything, but I can't think of anything better.)

[michaelfm1211]

@toofar I just pushed added a new commit to use {url:yank} instead. I agree, it looks much nicer than {yank_url}. I also like having the word "yank" in there so it reflects the name of the function in urlutils.py.

[toofar]

Cool, I'll symbolically approve it for now but try to grab a bit of @The-Compiler's time over the next week (no worries if they are too busy) to get another opinion since I don't feel too confident in this area.

Trying to recap:

• the goal of the PR is to generalize the query param stripping a bit so it applies in more places
• I asked if we could apply it in all cases!
• turns out there are some complicated places, in particular the {url} in command substitutions. There might be cases where you expect the verbatim URL to be used, for example when using that substitution as an argument to a userscript. So instead of changing {url} to have query params that you probably don't care about stripped, we are introducing a {url:yank} variable and changing the default ym and yM bindings to use it. The other option I can see is changing {url} then adding a {url:raw} variable to get an unstripped URL.
• it's not a hugely impactfull decision overall so if you don't have an opinion than that's fine too. We have a path forward

[The-Compiler]

Looks good all in all! I agree that {url:yank} is the way to go and {url} should be left untouched.

I added a couple of nitpicks, mostly on API design things.

[michaelfm1211]

@The-Compiler I've added those improvements. I agree, using keyword arguments is probably better. I think everything should be all good/ready now.

[michaelfm1211]

@The-Compiler Any update on this? Do you think this is ready to merge?

[toofar]

Hi @michaelfm1211, maybe check back in a week? @The-Compiler is busy moving house and currently we are trying to get a 3.0.1 release out (not with all the issues in that milestone, just as much as we can) to line up with the 6.5.3 Qt release (which should be this week). That's going to be mostly fixing Qt6 regressions, so that's what we are focusing on atm.

[michaelfm1211]

@The-Compiler Checking back on this a few months later. Is this PR still useful and/or in a good to merge?