Review Request 112023: Disable move/copy actions for non-writeable UMS collections.

Sun Sep 1 19:12:18 UTC 2013

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112023/#review39091
-----------------------------------------------------------

Hi, sorry for the delay replying, I had a king of work emergency. Please see some comments below.

src/core/collections/Collection.h
<http://git.reviewboard.kde.org/r/112023/#comment28829>

    I'd love if this could be named WritableState, as WriteState triggers something very different in my mind. Code is written once and read many times, it is therefore hopefully worth the effort.

src/core/collections/Collection.h
<http://git.reviewboard.kde.org/r/112023/#comment28828>

    I must say I don't like a method named isSomething() returning non-bool. We tend to adhere to Qt naming style and in Qt, isSomething() really means that Something is a bool.

    I suggest this:
    virtual WritableState writableState() const;

    I also suggest a convenience non-virtual method:
    bool isWritable() const;

    As most users don't care whether non-writability if permanent or temporary, they could continue using isWritable() without changes, which would shrink this patch. It should be however checked that no subclass implements it. (or, can we use C++11 final keyword already?)

    ^^^ the same applies to CollectionLocation too.

src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/112023/#comment28830>

    These fwd-declarations are no longer needed.

- Matěj Laitl

On Aug. 25, 2013, 11:09 p.m., Frank Meerkoetter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112023/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2013, 11:09 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> The fix in https://git.reviewboard.kde.org/r/111991/ fixes isWriteable() for UMS collections. This has the effect that if an UMS-collection is actually non-writeable the copy/move operations will be missing from the TreeView/Filebrowser context menus. There is no clue to the user what is going on.
> 
> This patch is introducing a new method to the Collection interface so we can differentiate between collections that are non-writeable by principle and collections that just happen to be non-writeable right now (for what ever reason).
> 
> I use this changed collection interface to disable move/copy actions (as opposed to hide) for non-writeable UMS-colllections. This should give the user at least a hint what is going on.
> 
> Unfortunately i haven't found a way to display a tooltip ("Currently readonly"). The action is offering an setTooltip() method but now effect was observed.
> 
> 
> Diffs
> -----
> 
>   src/browsers/CollectionTreeItemModel.cpp 1ac4463 
>   src/browsers/CollectionTreeView.cpp 6fad164 
>   src/browsers/filebrowser/FileView.cpp ad200eb 
>   src/core-impl/collections/db/sql/SqlCollectionLocation.h da1b2e5 
>   src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 3b55f66 
>   src/core-impl/collections/ipodcollection/IpodCollection.h 14fdd40 
>   src/core-impl/collections/ipodcollection/IpodCollection.cpp 36738d7 
>   src/core-impl/collections/ipodcollection/IpodCollectionLocation.h cc27e19 
>   src/core-impl/collections/ipodcollection/IpodCollectionLocation.cpp f8105f9 
>   src/core-impl/collections/ipodcollection/IpodMeta.cpp 6beca0a 
>   src/core-impl/collections/ipodcollection/IpodPlaylist.cpp 61c60ba 
>   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp dcef4c2 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h e40529f 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.cpp 603ceff 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp 4a419d5 
>   src/core-impl/collections/nepomukcollection/NepomukCollection.h c5d7a13 
>   src/core-impl/collections/nepomukcollection/NepomukCollection.cpp 77aef8e 
>   src/core-impl/collections/playdarcollection/PlaydarCollection.h 1a7ec89 
>   src/core-impl/collections/playdarcollection/PlaydarCollection.cpp 5c2349d 
>   src/core-impl/collections/support/FileCollectionLocation.h 9bd303e 
>   src/core-impl/collections/support/FileCollectionLocation.cpp 4b9b4b0 
>   src/core-impl/collections/support/TrashCollectionLocation.h 239a977 
>   src/core-impl/collections/support/TrashCollectionLocation.cpp 61c2e49 
>   src/core-impl/collections/umscollection/UmsCollection.cpp a73fd34 
>   src/core-impl/collections/umscollection/UmsCollectionLocation.h 6cfe4df 
>   src/core-impl/collections/umscollection/UmsCollectionLocation.cpp cef90d7 
>   src/core/collections/Collection.h 94d24d7 
>   src/core/collections/Collection.cpp 01a2e8c 
>   src/core/collections/CollectionLocation.h 7a1ed1e 
>   src/core/collections/CollectionLocation.cpp 2385570 
>   src/services/ServiceCollectionLocation.h cea3b84 
>   src/services/ServiceCollectionLocation.cpp d1cb0d8 
>   src/services/mp3tunes/Mp3tunesService.cpp ed2b006 
>   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.h 2b06cb4 
>   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.cpp 6aaa66d 
>   src/synchronization/MasterSlaveSynchronizationJob.cpp 1979701 
>   src/synchronization/OneWaySynchronizationJob.cpp 99f504c 
>   src/synchronization/UnionJob.cpp 8602044 
>   tests/core-impl/collections/db/sql/TestSqlCollectionLocation.cpp ab41db3 
>   tests/core/collections/CollectionLocationTest.cpp 720fa6a 
>   tests/core/collections/TestCollection.cpp 72a8b3b 
>   tests/synchronization/TestMasterSlaveSynchronizationJob.cpp b8b59c4 
>   tests/synchronization/TestOneWaySynchronizationJob.cpp ce7a2b5 
>   tests/synchronization/TestUnionJob.cpp 2343577 
> 
> Diff: http://git.reviewboard.kde.org/r/112023/diff/
> 
> 
> Testing
> -------
> 
> I have tested with an USB-stick that is writeable and another one that is non-writeable. I have also observed that for the FileBrowser the "Local collection" is still shown as active.
>  
> 
> 
> Thanks,
> 
> Frank Meerkoetter
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130901/5b2b41f0/attachment-0001.html>