Review Request 123888: [krunner] Bring back history

[Kai Uwe Broulik]

> On Mai 23, 2015, 11:21 nachm., Mark Gaiser wrote:
> > krunner/view.h, line 48
> > <https://git.reviewboard.kde.org/r/123888/diff/1/?file=370580#file370580line48>
> >
> >     const QStringList &history() const;
> >     
> >     Saves a copy and prevents users from manipulating this object.
> >     
> >     You do manipulate this, but that in the QML and you put it in a "var history" which probably copies it also in a javascript object?
> >     
> >     Or i'm wrong... In that case you can ignore this comment :)

QML cannot track changes inside JS objects, so I need to do an explicit assignment for it to notice hence the indirection.


> On Mai 23, 2015, 11:21 nachm., Mark Gaiser wrote:
> > lookandfeel/contents/runcommand/RunCommand.qml, lines 98-102
> > <https://git.reviewboard.kde.org/r/123888/diff/1/?file=370582#file370582line98>
> >
> >     Hmm, this looks interesting.
> >     Suppose my history looks like this:
> >     - plasmashell
> >     - konsole
> >     - dolphin
> >     
> >     now imagine i type "plasma". It should then show:
> >     - plasmashell
> >     
> >     Thus far it probably works fine.
> >     Now imagine i clear whatever i typed (so a clean inputfield). All without closing krunner! When i clear it the history probably shows one entry:
> >     - plasmashell
> >     
> >     What happened to the other two?
> >     Well, you've rewritten history when i typed a search query.
> >     And because you call "runnerWindow.history = history" it will call the C++ setHistory method which will overwrite your history value in the config.. Ouch!
> >     
> >     I could be wrong, but this is how i think it behaves when reading the code.
> >     I think you should use something with a tree. A radix tree would be the easiest, fastest and cheapest in memory, but that doesn't exist in Qt nor the C++ STL.

I don't get it. I only ever write to the history when you actually invoke a search result.


On Mai 23, 2015, 11:21 nachm., Kai Uwe Broulik wrote:
> > I'm curious (since i don't see that code here). Where do you add items to your history?

"The unshift() method adds new items to the beginning of an array"


- Kai Uwe


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123888/#review80757
-----------------------------------------------------------


On Mai 23, 2015, 10:01 nachm., Kai Uwe Broulik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123888/
> -----------------------------------------------------------
> 
> (Updated Mai 23, 2015, 10:01 nachm.)
> 
> 
> Review request for Plasma, KDE Usability and Vishesh Handa.
> 
> 
> Bugs: 335731
>     https://bugs.kde.org/show_bug.cgi?id=335731
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> This turns KRunner's TextField into an editable ComboBox to provide a history.
> 
> When a result is invoked, the query string is prepended to the history, query strings are only added once. ComboBox provides letter-by-letter auto completion.
> 
> 
> Diffs
> -----
> 
>   krunner/view.h 1ad5075 
>   krunner/view.cpp 8640e1d 
>   lookandfeel/contents/runcommand/RunCommand.qml 4c6eb30 
> 
> Diff: https://git.reviewboard.kde.org/r/123888/diff/
> 
> 
> Testing
> -------
> 
> Somehow I have a feeling it doesn't always save the history or nukes it at times. It also has some shortcomings due to ComboBox:
> 
> 1.) You cannot use the arrow keys to cycle between entries (when the popup's not opened) because arrow keys navigate through results
> 2.) forceActiveFocus() on the ComboBox will not activate the embedded TextField - when you had opened the popup there's a slight chance the input field won't get focussed I'll prepare a Qt patch for this.
> 3.) Before Qt 5.4.2 (not sure if my patch ended up in 5.4.1) pressing space in the edit combobox will open the popup, not insert a space (nasty show stopper)
> 4.) Plasma's edtiable ComboBox looks a bit strange imho
> 5.) Plasma's editable ComboBox doesn't support clearButtonShown
> 6.) Plasma's ComboBox has strange bullets and margins in it, that's probably a bug in Plasma Style (need to look what Desktop style does differently from us)
> 7.) ComboBox doesn't have a cursorPosition, I'll prepare a Qt patch for this.
> 
> 
> File Attachments
> ----------------
> 
> History popup
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/05/23/7ad7e5eb-4874-4f9f-9796-738fa2ac9ed5__krunnerhistory.png
> Auto completion
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/05/23/18714844-ef28-4cdd-af00-e6685caece9b__krunnerautocompletion.png
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150523/09e34766/attachment-0001.html>