[PATCH] KFilePlacesModel : separate storage for custom items and standard items
nf2 at scheinwelt.at
Wed Mar 12 01:56:48 GMT 2008
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
> - 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
> 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.
More information about the kde-core-devel