Text highlighting in the url bar simplified

Review Request #100154 - Created Nov. 16, 2010 and discarded

Jon Ander Peñalba
I've simplified text highlighting. I think the behaviour hasn't changed, but a second opinion is welcome :)

I've removed the Qt::escape (and updated the test accordingly) because I find it useless, but if it needs to be there for any reason there's no problem in putting it back.
The 'listitem_test' test passes.
Pierre Rossi
Benjamin Poulain
Andrea Diamantini
Jon Ander Peñalba
Review request changed

Change Summary:

Special symbols removed


Revision 2 (+59 -67)

Show changes

Jon Ander Peñalba
I think that this solves the problem. I don't know if I've added all the symbols that cause trouble, but if not, it's really simple to update :)
  1. In my opinion, it is just the wrong way to solve the problem. You still have not addressed the case where you match "XX" in a string of "XXXXXXXXXXXXX".
    In you last patch, you replace special symbols, but those can appear in an URL, you want to match them. If you want to escape all the control character in a meaningful way, your code will end up being bigger than the previous code.
    The objective of you patch is "Text highlighting in the url bar simplified", the new code is neither simplified, neither correct.
  2. I think it's simpler and way more readable than our current solution.
    I don't understand the problem of "XX" in "XXXXX". The result is "<b>XX</b><b>XX</b>X" and that's what's supposed to be.
    I know that hard-coding all the symbols and removing them from the string is normally a bad solution, but in this case I think we have more to gain, the previous code is quite hard to follow.
  3. If I take a site like http://www.snowandrock.com/all/ski/fcp-category/home I might want to look it up in the history by typing "snow+rock" and this currently doesn't work with the regexp approach.
    as for the </b><b> I don't really like it, if you type X then it'd result in <b>X</b><b>X</b><b>X</b><b>X</b><b>X</b>, even though it doesn't matter that much, that's a bit of overhead compared to <b>XXXXX</b>.
    Weighing the pros and cons, I think the regexp-based solution is going to become as unreadable as the current implementation quite soon at this rate, and I fear it might never be as robust.
    I agree that the current function used to do that can seem somewhat cryptic, and I take the blame for it, I should probably write a comment block there to explain what it does, since it's probably something that's bound to be changed and/or fine tuned in the future.