GNOME Bugzilla – Bug 706724
Bold matches against search string in search popup
Last modified: 2013-08-30 14:37:37 UTC
To show better why a search result was included, lets bold the substring that matches against the search string.
Created attachment 253025 [details] [review] MainWindow: Bold string matches in search popup
Review of attachment 253025 [details] [review]: Otherwise fine! ::: src/mainWindow.js @@ +235,3 @@ + _boldMatch: function(description) { + let searchString = this._searchEntry.get_text().toLowerCase(); Could we do this once? Not a huge thing but not necessary to run toLowerCase each time. @@ +240,3 @@ + if (index !== -1) { + let substring = description.substring(index, + index + searchString.length); Please add a comment on why we need to do substring + replace instead of just replace. I understand it but would be good for the next guy to also understand. :)
Created attachment 253026 [details] [review] MainWindow: Bold string matches in search popup v2
Created attachment 253027 [details] [review] MainWindow: Bold string matches in search popup v3
Created attachment 253029 [details] [review] MainWindow: Bold string matches in search popup v4 Added escaping of markup and some renaming of vars.
Review of attachment 253029 [details] [review]: ::: src/mainWindow.js @@ +239,3 @@ + + if (index !== -1) { + let substring = description.substring(index, Can we be sure that index remains the same regardless of case in ever locale? @@ +253,3 @@ + // Lower case to match case insensitive + let searchStringLower = this._searchEntry.get_text().toLowerCase(); Another place where use of props would make for slightly more readable code.
Review of attachment 253029 [details] [review]: ::: src/mainWindow.js @@ +239,3 @@ + + if (index !== -1) { + let substring = description.substring(index, I have no idea :( There is a toLocaleLowerCase() but I'm not sure it helps. @@ +253,3 @@ + // Lower case to match case insensitive + let searchStringLower = this._searchEntry.get_text().toLowerCase(); Agreed!
Review of attachment 253029 [details] [review]: ::: src/mainWindow.js @@ +239,3 @@ + + if (index !== -1) { + let substring = description.substring(index, Ok so reading on it, in order for the users locale to have affect in you should use the toLocaleLowerCase. And for Turkish it will have an effect on the length of the different cases. So we should use toLocaleLowerCase that should behave the same as toLowerCase otherwise. Good Catch. (reference: http://code.google.com/p/v8/issues/detail?id=2196, even tho that is about v8 not handling it correctly)
Created attachment 253260 [details] [review] MainWindow: Bold string matches in search popup * use javascript String.bold() method to insert <b> and </b> * use toLocaleLowerCase to be safe in Turkish locale
Review of attachment 253029 [details] [review]: ::: src/mainWindow.js @@ +239,3 @@ + + if (index !== -1) { + let substring = description.substring(index, gjs> "İ".length 1 gjs> "İ".toLowerCase().length 1 gjs> "İ".toLocaleLowerCase().length 2 That makes me think that we actually should not use toLocaleLowerCase().
Created attachment 253345 [details] [review] MainWindow: Bold string matches in search popup v6 Thanks for investigating! * Use toLowerCase * rebased against master
Created attachment 253379 [details] [review] MainWindow: Bold string matches in search popup v7 * rebased against bug#706653
Review of attachment 253379 [details] [review]: Looks good but I didn't ask for freeze exception for this one. :(
Created attachment 253508 [details] [review] MainWindow: Bold string matches in search popup Rebased on current git master.
Review of attachment 253508 [details] [review]: ::: src/mainWindow.js @@ +265,1 @@ + model.clear(); This happens in onSearchActivate() now
Created attachment 253586 [details] [review] MainWindow: Bold string matches in search popup No need to clear model twice.
Review of attachment 253586 [details] [review]: If not clearing the model is the only change and this works and is on top of git master (hopefully you found my rebasing useful? :P), ACK
Attachment 253586 [details] pushed as 8102fe2 - MainWindow: Bold string matches in search popup
Created attachment 253604 [details] [review] MainWindow: Abort if no search results was found Commit: 8102fe2 MainWindow: Bold string matches in search popup, removed the check for when no search results were found. Lets reinstate it.
Comment on attachment 253604 [details] [review] MainWindow: Abort if no search results was found Attachment 253604 [details] pushed as 34daf30 - MainWindow: Abort if no search results was found