[PATCH] KFilePlacesModel : separate storage for custom items and standard items

[nf2]

Kevin Ottens wrote:
> Le Sunday 09 March 2008, nf2 a écrit :
>   
>> Here is an experimental patch for KFilePlacesModel, which separates the
>> storage for the custom items and the standard ("root") items.
>>     
>
> OK, let's start with the cosmetic comments:
>
>  - I wouldn't call the standard items "root" in the code, that's IMO 
> confusing, I'd expect them to be at the root of the filesystem and that's not 
> the case. I'd call them "system", "vendor", or something like this.
>   

Yeah. I'm not happy with "root" as well. Thought of calling them 
"standard"-items. "System" or "vendor" sounds good as well.

>  - In KFilePlacesModel if you keep two KBookmarkManager you should probably 
> give them better names (bookmarkManager vs bookmarkManagerRoot lacks of 
> distinction).
>
>  - reloadAndSignals(bool) should be two methods, or take the bookmark manager 
> as argument instead.
>   

I agree - would look better.

>  - I noticed the KStandardDirs().localxdgdatadir(), I'm wondering but maybe 
> we're lacking a "xdgdata" resource name for KStandardDirs, it might be worth 
> to add maybe?
>
>  - Also be careful at the coding style and the indentation, I spotted and 
> indentation mistake line 166 or your patch, and the if() in 
> reloadAndSignals() doesn't comply to kdelibs coding standard.
>
>   
>> The custom items go to ~/.local/share/filemgr-shortcuts.xbel, that they
>> can be shared with other desktops, and the root items are stored in
>> ~/.kde4/share/apps/kfileplaces/rootbookmarks.xml.
>>     
>
> I'd put ~/.local/share/filemgr-shortcuts.xbel somewhere else to avoid 
> cluttering ~/.local/share, a subfolder in apps perhaps?
>   

Well - the proposed location in the spec is 
~/.local/share/shortcuts.xbel. I thought filemgr-shortcuts.xbel is more 
self explanatory. Regarding the apps subdir i'm not sure, because that's 
not an app specific thing...


>   
>> This patch depends on my previous KBookmarks patch (to enable KDirWatch
>> monitoring for external bookmarks).
>>     
>
> OK, I applied your patches, and it totally blows the unit tests for the class, 
> so please rework your patch and ensure it at least passes the tests.
>
>   
>> I still have a little problem that <IsHidden> is not always written to
>> the files correctly, but i can't figure out where the bug is.
>>     
>
> Well, there's also a few more issues (actually catched by the tests). For 
> instance I can't reorder my places anymore, 

This might be because my KDirWatch based bookmark-manager mode reveals a 
bug in KFilePlacesModel (as it triggers file-reloads more often): The 
KBookmark member in class KFilePlacesItem pointing to a "detached" 
QDomDocument after a file reload (because a new QDomDocument is created 
in KBookmarkManager::parse()). Therefore you can't delete or move those 
items anymore. You can easily reproduce this problem by working with 
Dolphin and the file-dialog simultaneously (even without my patch).

I guess this can only be fixed by just keeping the bookmark-id*) in the 
KFilePlacesItem instead of the KBookmark. Or by re-assigning all the 
KBookmark's in _k_reloadBookmarks().

*) Btw, i would prefer using the URL as id, as this would be compliant 
to the spec.

> the ones living in .local are 
> necessarily at the end of the list.
>   

Reordering across the two bookmark files would probably require a 
"position" tag for merging the list.

Regards,
Norbert