Review Request 129673: [recentlyPLayedList] Add keyboard shortcuts

[Harald Sitter]

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



QUrl has static functions to convert a qstringlist and vice versa, makes for less code.

Two thoughts on this though:

1. The PathEntry functions make little sense here. All they do is inject $HOME which makes no difference here unless we assume users tend to rename their home a lot. It might well be wise to change to the regular read/write functions. We'd then have a template reader [1] so we can read into a `QList<QUrl>` directly and write from one as well. We lose nothing but get smoother code and it would be faster as it doesn't have to substitute $HOME all the time. So if you are interested migrating away from the Path entries would be a good project.
2. Additionally if you fancy some work on framworks, I think KConfigGroup would benefit from having `readPathEntry(char*, QList<T> &aDefault)` as a template function like the one at [1]. It would behave like the QStringList function but then use a default constructor `T(string);` to put the expanded string into a template object. In terms of QUrl that would allow us to get rid of excess list conversion by simply using a `QList<QUrl>` template default and having KConfigGroup feed `QUrl(string)` objects into that.

Eitherway, if you change to the QUrl helpers for list conversion this should be good to ship.

[1] https://api.kde.org/frameworks/kconfig/html/classKConfigGroup.html#af06d0a8cf16282ba585ef78699b21792


src/app/recentlyPlayedList.cpp (line 46)
<https://git.reviewboard.kde.org/r/129673/#comment67952>

    Why is this change needed?
    
    Also, for future reference. Please init one object at a time
    
    ```
    clear = new
    clear.setText
    ...
    connect(clear, ...)
    
    remove = new
    remove.setText
    ...
    connect(clear, ...)
    ```
    
    It's easier to read that way.



src/app/recentlyPlayedList.cpp (line 97)
<https://git.reviewboard.kde.org/r/129673/#comment67953>

    I do not think this can happen, can it?



src/app/recentlyPlayedList.cpp (line 101)
<https://git.reviewboard.kde.org/r/129673/#comment67957>

    We can do this way simpler
    
    ```
    auto list = QUrl::fromStringList(configGroup->readPathEntry("Recent Urls", QStringList()));
    ```
    
    to convert the `qstringlist` to a `qlist<qurl>` and 
    
    ```
    configGroup->writePathEntry("Recent Urls", QUrl::toStringList(list));
    ```
    
    to convert it back.



src/app/stateChange.cpp (line 150)
<https://git.reviewboard.kde.org/r/129673/#comment67958>

    Same as with the other code, convert->remove->write


- Harald Sitter


On Dec. 20, 2016, 5:06 a.m., Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129673/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2016, 5:06 a.m.)
> 
> 
> Review request for KDE Multimedia and Harald Sitter.
> 
> 
> Repository: dragon
> 
> 
> Description
> -------
> 
> + Correct unspected behavior QString operator== not work for toDisplayString() e.g. file:$HOME/???????/FPS_test_1080p23.976_L4.1.mkv
> 
> 
> Diffs
> -----
> 
>   src/app/recentlyPlayedList.cpp 2c25e7f 
>   src/app/stateChange.cpp 7bf4038 
> 
> Diff: https://git.reviewboard.kde.org/r/129673/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> Actions
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/12/20/fad5b9ab-9ffb-4220-a0f4-5a25845c6507__Screenshot_20161220_070541.png
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20161220/31b4fee2/attachment.htm>