D7700: Show list of tags in PlacesView

[Nicolas Fella]

nicolasfella created this revision.
nicolasfella added a project: Dolphin.

REVISION SUMMARY
Similar to https://phabricator.kde.org/D7668 but instead of a single entry for tags:/ a new "Tags" group with all available tags is created.

Future plans:
Limit to most popular tags
Generally improve tags in Dolphin

TEST PLAN
Assign some tags to files, force a reindex of the files and restart Dolphin to see the result

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/panels/places/placesitem.cpp
src/panels/places/placesitem.h
src/panels/places/placesitemmodel.cpp
src/panels/places/placesitemmodel.h
src/panels/places/placespanel.cpp

To: nicolasfella, #dolphin
Cc: #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Nicolas Fella]

nicolasfella added a reviewer: KDE Applications.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications
Cc: #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Alexey Min]

alexeymin added a comment.


Compiles OK and shows `All tags` item, but with no children, how do I force a reindex?

INLINE COMMENTS

placesitemmodel.cpp:1195
+ m_systemBookmarks.append(SystemBookmarkData(QUrl(tagsUrlBase),
+ QStringLiteral("tag"),
+ I18N_NOOP2("KFile System Bookmarks", "All tags")));

weird indentation

placesitemmodel.cpp:1199
+ for (QString tag: m_tags) {
+ m_systemBookmarks.append(SystemBookmarkData(QUrl(tagsUrlBase+tag),
+ QStringLiteral("tag"),

indentation

placesitemmodel.cpp:1206
+
+void PlacesItemModel::addTag(KIO::Job* job, const KIO::UDSEntryList& entries )
+{

extra space

placesitemmodel.cpp:1213
+ const KIO::UDSEntry& entry = *it;
+ QString name = entry.stringValue( KIO::UDSEntry::UDS_NAME );
+

extra space

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Nicolas Fella]

nicolasfella added a comment.

Post by Alexey Min
Compiles OK and shows `All tags` item, but with no children, how do I force a reindex?

You can use "balooctl check". I don't know if there is a way to reindex a single file

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Alexey Min]

alexeymin requested changes to this revision.
alexeymin added a comment.
This revision now requires changes to proceed.


I've fixed my baloo, and tested, it works! Nice feature :)
gcc-5.4.0 gave a few warnings in a newly added code though, one about member initialization order, another about an unused parameter:

INLINE COMMENTS

placesitemmodel.cpp:84
+ m_storageSetupInProgress(),
+ m_tags()
{

warning: ‘PlacesItemModel::m_storageSetupInProgress’ will be initialized after [-Wreorder]
warning: ‘QList<QString> PlacesItemModel::m_tags’ [-Wreorder]
swap m_tags() with m_storageSetupInProgress() ?

placesitemmodel.cpp:1206
+
+void PlacesItemModel::addTag(KIO::Job* job, const KIO::UDSEntryList& entries )
+{

warning: unused parameter ‘job’ [-Wunused-parameter]
Q_UNUSED(job) ?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Alexey Min]

alexeymin added a comment.


I'm not sure, but it looks like first dolphin's startup time has increased, can it be caused by this? Maybe because of baloo or something?
Maybe I'm wrong, will test again tomorrow

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Nicolas Fella]

nicolasfella added a comment.


Not sure about Dolphin in general, but the Places Panel definitely takes longer to load

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Alexey Min]

alexeymin added a comment.

Post by Nicolas Fella
Not sure about Dolphin in general, but the Places Panel definitely takes longer to load

Yes, it is only Places panel. But I could probably live with that

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Nicolas Fella]

nicolasfella marked 6 inline comments as done.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Nicolas Fella]

nicolasfella updated this revision to Diff 19401.
nicolasfella added a comment.


Fixed indentation and warnings

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=19212&id=19401

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/panels/places/placesitem.cpp
src/panels/places/placesitem.h
src/panels/places/placesitemmodel.cpp
src/panels/places/placesitemmodel.h
src/panels/places/placespanel.cpp

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Alexey Min]

alexeymin accepted this revision as: alexeymin.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Nicolas Fella]

nicolasfella edited the summary of this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros, emmanuelp

[Emmanuel Pescosta]

emmanuelp added a comment.


Thanks for the patch!

I think that we should show only a limited number of tags with an additional `Show All` (or something like that) which opens `tags://`.

INLINE COMMENTS

placesitemmodel.cpp:96
+
+ // if tags are supported load them before loading the bookmarks
+ if (KProtocolInfo::isKnownProtocol(QStringLiteral("tags"))) {

Why?

placesitemmodel.cpp:1214
+ const KIO::UDSEntry& entry = *it;
+ QString name = entry.stringValue( KIO::UDSEntry::UDS_NAME);
+

Use the URL instead.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

emmanuelp wrote in placesitemmodel.cpp:96
Why?

Loading the Tags takes a while because of the async ListJob. I want to have all m_systemBookmarks items loaded when calling loadBookmarks() to use the same codepath for all items. This could be implemented otherwise if desired

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.

Post by Emmanuel Pescosta
Thanks for the patch!
I think that we should show only a limited number of tags with an additional `Show All` (or something like that) which opens `tags://`.

What do you think would be a reasonable number of tags? 5, 10, 20, ..? Make it configurable?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


FWIW, Apple's Finder already has this exact feature, and shows 7 tags by default, plus an "All Tags" item below them.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.


I was thinking about colored tags as well. Users can already change the icons of the tags in the sidebar. Maybe we could provide some "colored tag" icons. Making the tagged files colored would be worth trying, but is out of scope for this patch

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


Colored tags would be ideal. Agreed that it's out of scope for this patch, but good to work towards nonetheless.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

emmanuelp wrote in placesitemmodel.cpp:1214
Use the URL instead.

Do you mean UDS_URL instead of UDS_NAME? Why?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella updated this revision to Diff 19411.
nicolasfella added a comment.


Show max 8 (because we're better than macOS ;) ) Tags

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=19401&id=19411

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/panels/places/placesitem.cpp
src/panels/places/placesitem.h
src/panels/places/placesitemmodel.cpp
src/panels/places/placesitemmodel.h
src/panels/places/placespanel.cpp

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Elvis Angelaccio]

elvisangelaccio added a comment.


What happens if I don't have any tag? Will it show "All tags" without anything below?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.

Post by Elvis Angelaccio
What happens if I don't have any tag? Will it show "All tags" without anything below?

That's excactly what happens

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella updated this revision to Diff 19423.
nicolasfella added a comment.


Only show "All tags" if there are any tags

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=19411&id=19423

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/panels/places/placesitem.cpp
src/panels/places/placesitem.h
src/panels/places/placesitemmodel.cpp
src/panels/places/placesitemmodel.h
src/panels/places/placespanel.cpp

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Emmanuel Pescosta]

emmanuelp added a comment.

Post by Nicolas Fella
What do you think would be a reasonable number of tags? 5, 10, 20, ..? Make it configurable?

Please avoid configurable options as much as possible ;)

It seems that there is already a consensus about a reasonable number of tags -> so 8 ;)

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Emmanuel Pescosta]

emmanuelp added inline comments.

INLINE COMMENTS

nicolasfella wrote in placesitemmodel.cpp:1214
Do you mean UDS_URL instead of UDS_NAME? Why?

Ups mixed things up. `UDSEntry` doesn't have a `url()` method, only `KFileItem` has it.

Why?

The problem is that `UDS_URL` may be different from `baseUrl + UDS_NAME`, *depending* on the implementation of the ioslave. `KFileItem` handles this by checking if `UDS_URL` is set or not and handles it using two different code paths. But a user of KIO shouldn't depend on such implementation details, otherwise it may break in future if something changes in the ioslave.

I also think that we should use a `KDirLister` instead because:

- It gives us `KFileItems`
- We can react to `itemsAdded` and `itemsDeleted` signals -> shows new tags without restarting Dolphin

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Christoph Feck]

cfeck added a comment.

Post by Emmanuel Pescosta
It seems that there is already a consensus about a reasonable number of tags -> so 8 ;)

Which 8? Most used or recently used?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella updated this revision to Diff 19465.
nicolasfella added a comment.


Use KCoreDirLister (thanks @emmanuelp). It simplified the whole patch and fixed a problem with newly appearing bookmarks (e.g. when plugging in a USB-Stick). Right now the first 8 Tags are shown, but showing the most (recent) used would sure be better. Any ideas on how to implement this?

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=19423&id=19465

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/panels/places/placesitem.cpp
src/panels/places/placesitem.h
src/panels/places/placesitemmodel.cpp
src/panels/places/placesitemmodel.h
src/panels/places/placespanel.cpp

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella marked 2 inline comments as done.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


Showing the most recently used or most popular tags presents a usability issue since the list will be constantly changing and re-ordering itself, which impedes muscle memory and makes the software seem unstable. I would prefer that it show the tags in the order that they are created, and allow them to be re-ordered as normal--including from the "show more tags" view.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella updated this revision to Diff 19495.
nicolasfella added a comment.


Use url() instead of urlbase+name

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=19465&id=19495

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/panels/places/placesitem.cpp
src/panels/places/placesitem.h
src/panels/places/placesitemmodel.cpp
src/panels/places/placesitemmodel.h
src/panels/places/placespanel.cpp

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella marked 3 inline comments as done.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

placesitemmodel.cpp:84
+ m_tags(),
+ m_dirLister()
{

Remove it, it's pointer type

placesitemmodel.cpp:98
+ if (KProtocolInfo::isKnownProtocol(QStringLiteral("tags"))) {
+ connect(m_dirLister, &KCoreDirLister::itemsAdded, this, &PlacesItemModel::addTags);
+ m_dirLister->openUrl(QUrl(m_tagsUrlBase));

Remove tag?

placesitemmodel.cpp:694
+ if (item->groupType()==PlacesItem::TagsType){
+ continue;

a == b

placesitemmodel.cpp:1188
+
+void PlacesItemModel::addTags(const QUrl& directoryUrl, const KFileItemList& items)
+{

Name should be addTag, because you add it one by one.

placesitemmodel.cpp:1191
+ Q_UNUSED(directoryUrl);
+ int maxTags = 8;
+ int numTags = 0;

const

placesitemmodel.cpp:1192
+ int maxTags = 8;
+ int numTags = 0;
+

Remove it, use m_tags.size instead.

placesitemmodel.cpp:1194
+
+ for (KFileItem item: items) {
+ QString name = item.name();

This is a copy, use const kFileItem &item instead

placesitemmodel.cpp:1195
+ for (KFileItem item: items) {
+ QString name = item.name();
+

const, make all unmodified variables const ones.

placesitemmodel.cpp:1197-1202
+ if (m_tags.isEmpty()) {
+ PlacesItem* allTagsItem = createSystemPlacesItem(SystemBookmarkData(QUrl(m_tagsUrlBase),
+ QStringLiteral("tag"),
+ I18N_NOOP2("KFile System Bookmarks", "All tags")));
+ appendItemToGroup(allTagsItem);
+ }

Move out from loop, it's true only one time

placesitemmodel.h:291
+ QList<QString> m_tags;
+ QString m_tagsUrlBase = QStringLiteral("tags:/");
+ KCoreDirLister* m_dirLister;

Make it const and you can initialize it in constructor time.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella marked 9 inline comments as done.
nicolasfella added inline comments.

INLINE COMMENTS

anthonyfieroni wrote in placesitemmodel.cpp:98
Remove tag?

KCoreDirLister's itemsDeletedSignal is not emited when Tags are removed via Information Panel. In the tags IOSlave delete is not implemented.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella updated this revision to Diff 19502.
nicolasfella added a comment.


Addressed comments

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=19495&id=19502

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/panels/places/placesitem.cpp
src/panels/places/placesitem.h
src/panels/places/placesitemmodel.cpp
src/panels/places/placesitemmodel.h
src/panels/places/placespanel.cpp

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.


Any further objections/issues/comments?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


Looks good to me. @anthonyfieroni?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Anthony Fieroni]

anthonyfieroni added a comment.


@emmanuelp should accept it.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


@emmanuelp Ping! It would be great to get your blessing so we can land this.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin
Cc: anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


FWIW, I've been testing this and it works really well. Nice feature.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Mark Gaiser]

markg added a comment.


Sorry for acting like a "protect the places panel" guy again.. but yeah, again :)
Please make a Panel for tags, not slapping it into the places panel. The macOS finder example was shown, that exact result can be achieved by putting it into the places panel, but also by putting it in it's own panel!
Making it's own panel for it is much cleaner and maintainable.

I know it's a lot more work to make a new panel then to add something to an existing one. But tags **really** have nothing to do with places. They are two different concepts.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


If we make a new panel, we incur the following disadvantages:

1. Lots of extra work, increasing the chance that it won't get done. We already have working code to add it to the Places panel.

2. Assuming we make a new Tags panel, if we don't make it visible by default, almost nobody will ever see it, negating the purpose of adding the feature and keeping the overall Tags feature very un-discoverable.

3. If we *do* make the new Tags panel visible by default, it will compete for vertical space currently used by the Places and Folders panels, and people will probably close it if it's visible by default, and not open it in the first place if it's not.

I know that technically Tags aren't places, but the advantage of putting this in Places is that people will actually see it and use it. If we put it somewhere else, especially in a not-visible-by-default panel, I worry that the work will be essentially wasted.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Mark Gaiser]

markg added inline comments.

INLINE COMMENTS

placesitemmodel.cpp:91
m_bookmarkManager = KBookmarkManager::managerForExternalFile(file);
+ m_dirLister = new KCoreDirLister();

Set a parent (you can use "this" i think), then you don't have to delete it later on (the parent -> child structure will take care of it).
+ move it up to the constructor initializers.

Also, rename this variable. It's not a "dirLister" even though you use a K(Core)DirLister object. You listen for tags. Call it something like m_tagsLister or so.

placesitemmodel.cpp:118
m_bookmarkedItems.clear();
+ delete m_dirLister;
}

Remove this line if initializing m_dirLister in the constructor initialize section.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Mark Gaiser]

markg added a comment.

Post by Nathaniel Graham
1. Lots of extra work, increasing the chance that it won't get done. We already have working code to add it to the Places panel.
2. Assuming we make a new Tags panel, if we don't make it visible by default, almost nobody will ever see it, negating the purpose of adding the feature and keeping the overall Tags feature very un-discoverable.
3. If we *do* make the new Tags panel visible by default, it will compete for vertical space currently used by the Places and Folders panels, and people will probably close it if it's visible by default, and not open it in the first place if it's not.
I know that technically Tags aren't places, but the advantage of putting this in Places is that people will actually see it and use it. If we put it somewhere else, especially in a not-visible-by-default panel, I worry that the work will be essentially wasted.

1. That is the wrong mentality. Just the fact that "it works in places" doesn't mean that it should be there. It means the current path is the easiest one to get the fastest results, but refactoring later on would still be needed.
2. I'd like to propose to make a new panel **and** make it visible by default! You'd get my vote for that (granted that it is performant).
3. That argument is moot. The tags would always compete for vertical space regardless of it's implementation. Also, (if enabled obviously) it would always be visible regardless of vertical size. Dolphin has no mechanism to auto hide panels of the space becomes too small, it just adds scrollbars if it does.

Lets make this great and expandable for the future (aka, add the colors later on and make it look fancy).

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Mark Gaiser]

markg added a comment.


Just curious, i have some files with tags (just added them). Baloo is running, but tags:/ is empty...
System monitor shows a baloo_file running at 100% cpu...

Again, same with recentdocuments, if stuff like this is added by default, it needs to work just fine in the normal usecases, otherwise it will cause bug reports which will in term cause the tags part to be disabled by default. Yes, chicken and egg problem plays here as well.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.


I would be fine with moving it to a separate panel. If we do that, the PlacesPanel, the TagsPanel and a (hypothetical, but I saw people voting for it) BookmarksPanel would share a lot of code. Basically it would be the same View and Controller with different Models. I would suggest to get rid of that redundancy by sharing the View and Controller.

Post by Mark Gaiser
Again, same with recentdocuments, if stuff like this is added by default, it needs to work just fine in the normal usecases, otherwise it will cause bug reports which will in term cause the tags part to be disabled by default. Yes, chicken and egg problem plays here as well.

I'm working on improving the tags ioslave. https://phabricator.kde.org/D7855 was a first step on this.

Post by Mark Gaiser
Just curious, i have some files with tags (just added them). Baloo is running, but tags:/ is empty...
System monitor shows a baloo_file running at 100% cpu...

I'm not an expert on baloo, but waiting and klicking Refresh in dolphin should make it work

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


If we can make it a separate panel and overcome some of the disadvantages of having multiple panels. I'm fine with it. That means:

1. Make the Tags panel visible by default
2. Include a visible separator between panels (https://bugs.kde.org/show_bug.cgi?id=384999)

I can work on https://bugs.kde.org/show_bug.cgi?id=384999.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Mark Gaiser]

markg added a comment.

Post by Nicolas Fella

Post by Mark Gaiser
Again, same with recentdocuments, if stuff like this is added by default, it needs to work just fine in the normal usecases, otherwise it will cause bug reports which will in term cause the tags part to be disabled by default. Yes, chicken and egg problem plays here as well.

I'm working on improving the tags ioslave. https://phabricator.kde.org/D7855 was a first step on this.

Nice! :)

Post by Nicolas Fella

Post by Mark Gaiser
Just curious, i have some files with tags (just added them). Baloo is running, but tags:/ is empty...
System monitor shows a baloo_file running at 100% cpu...

I'm not an expert on baloo, but waiting and klicking Refresh in dolphin should make it work

Ahh, that indeed seems to be the case. Lots of waiting that is.
Memory usage also shoots up when baloo_file and baloo_file_extractor are running (those two combined take up ~3.5GB of memory here at the moment).
Tag changes don't seem to immediately be propagated to tags:/ .. Far from it. More like half an hour or so, hehe.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Mark Gaiser]

markg added a comment.

Post by Nathaniel Graham
1. Make the Tags panel visible by default
2. Include a visible separator between panels (https://bugs.kde.org/show_bug.cgi?id=384999)
I can work on https://bugs.kde.org/show_bug.cgi?id=384999.

That would be my preference indeed. Be sure to check with Emmanuel for this as well.
As for No. 2; That will get you into style issues i think. And when no scrollbar is visible, you probably don't want to show the separator as there likely is a title visible anyhow. Perhaps if the title disappears show a separator line..

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Christoph Feck]

cfeck added a comment.


Ideally, the title does not scroll away at all.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


The title is only visible when the panels are unlocked. But in fact, the problem goes away when panels are unlocked; it's only a problem when locked. I'm working on a patch that improves this when panels are locked. Either way, we should continue that discussion in https://bugs.kde.org/show_bug.cgi?id=384999.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Mark Gaiser]

markg added a comment.

Post by Nathaniel Graham
The title is only visible when the panels are unlocked. But in fact, the problem goes away when panels are unlocked; it's only a problem when locked. I'm working on a patch that improves this when panels are locked. Either way, we should continue that discussion in https://bugs.kde.org/show_bug.cgi?id=384999.

I vaguely remember that being an issue in the Places panel as well from a couple years ago (no title when locked). I think the title was then added to the places panel. That now does give you the odd double "Places" when the panels aren't locked.
Imho, it would be fine if you add a title in your new panel just like places has one. So in unlocked mode your "Tags" panel will have "Tags" two times below one another, just like "Places" :) Lets call it consistency, hehe.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


Seems like a simple solution is to always show the title. That way, it wouldn't scroll away and you would still see it when locked. We would have to remove the redundant title from the Places panel with this, but that's simple enough.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


I've submitted a patch to Breeze that resolves the usability issues with having multiple adjacent DockWidget panels: https://phabricator.kde.org/D7957

Merging that (or otherwise fixing the issue) will eliminate my objections to putting this in a Panel that's visible by default.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella planned changes to this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, akrutzler, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


@nicolasfella, where are we at with this one? As much as poor @markg objects (and I don't totally disagree), I think it does still make sense to put Tags in the Places panel unless and until we get the proposed new design of making everything a separate panel and putting all panels into a single scrollview. Until that happens (and I do support it), let's just use the Places panel for now.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, spoorun, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.


IMHO we can merge it for now (maybe I will have to rebase) I like the idea of separate panels and I'm working on something similar. I try to extract generic pieces from the PlacesPanel to an abstract ListPanel that can be extended to create all kinds of panels we want. I'm not done yet but I hope I can show you something soon

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, spoorun, navarromorales, firef, andrebarros

[Elvis Angelaccio]

elvisangelaccio added a subscriber: renatoo.
elvisangelaccio added a comment.


I'd suggest to wait for the effort of upstreaming the places model in KIO (see @renatoo 's revisions), before merging this.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, spoorun, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


Agreed, that makes sense. Then it would show up in file dialogs too.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, spoorun, navarromorales, firef, andrebarros

[Michael Heidelbach]

michaelh added a comment.


I must have misunderstood something in this discussion. The functionality is there already.
Activate folderpanel when navigating to tags:// you get.
F5683309: dolphin-flags.png <https://phabricator.kde.org/F5683309>
Why an extra panel?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: michaelh, renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros

[Michael Heidelbach]

michaelh added a comment.


@alexeymin

Post by Alexey Min
how do I force a reindex?

Just in case...

$ balooctl clear <file>
$ balooctl index <file>

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: michaelh, renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros

[Störm Poorun]

spoorun added a comment.

Post by Michael Heidelbach
I must have misunderstood something in this discussion. The functionality is there already.
Activate folderpanel when navigating to tags:// you get.
F5683309: dolphin-flags.png <https://phabricator.kde.org/F5683309>
Why an extra panel?

Because of usability.
Majority of users wouldn't know that.
It adds extra work.
It's more efficient to see the tags (if you're using them) consistently, without extra work, to drag to and from them etc... (if you don't use them, you close the panel).

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Störm Poorun]

spoorun added a comment.

Post by Emmanuel Pescosta

Post by Nicolas Fella
What do you think would be a reasonable number of tags? 5, 10, 20, ..? Make it configurable?

Please avoid configurable options as much as possible ;)
It seems that there is already a consensus about a reasonable number of tags -> so 8 ;)

I disagree. In my organisation we use tags, instead of folders. We only want to see the tags panel. We don't want to have to click 'see more' each time.

Adding a discrete configuration option in loses nothing and gains everything for those who regularly use tags as a means of semantic organisation (better than folders).

Please at least have a configurable number shown option (including show all).

We don't restrict the number of folders shown in the other panels!

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Störm Poorun]

spoorun added a comment.


It should be noted that Baloo seems to be very slow in updating tags to the index. So users will not see their tagged items appear under the relevant tag for some time after tagging.
Trying to file a bug, but seeing if anyone knows about this already first (may be an indexing issue?)

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Michael Heidelbach]

michaelh added a comment.


Tried https://phabricator.kde.org/F5?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Störm Poorun]

spoorun added a comment.

Tried <https://phabricator.kde.org/F5>?

Yes. No change. And tried on different devices.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


@nicolasfella, what are the next steps here?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, markg, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.


The patch can't stay as-is, because the model was replaced by the one from KIO. We could either include this patch into KIO (which would raise concerns about having too much stuff there) or make it a separate panel. I once planned to extract common stuff from places panel and tags panel to a common ancestor, but that's quite some work. I'll try to put together a standalone Tags Panel soon

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


FWIW I'm still in favor of putting this in the Places panel via KIO, especially now that we have support for showing and hiding categories. If we make this its own new panel, I'd want to push for adding it to the default layout in Dolphin, or else nobody will ever see it and the work will be wasted.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Mark Gaiser]

markg added a comment.

Post by Nathaniel Graham
FWIW I'm still in favor of putting this in the Places panel via KIO, especially now that we have support for showing and hiding categories. If we make this its own new panel, I'd want to push for adding it to the default layout in Dolphin, or else nobody will ever see it and the work will be wasted.

And that very reasoning is the **exact** reason why i was against the "cram everything in the places panel" movement.
But that's done now, and for good reasons. I know.
But this does kinda kill the multiple panel idea in dolphin and effectively makes the Places panel "the" panel.

Having said that, i don't care enough to fix it. I haven't provided patches (like you folks have) nor do i have the time for it anyhow.
I'm fine with it either way.

Note that i do very much like the results it brings!
Note 2: Why do i still get mails from this thread when i unsubscribed.. Weird phabricator...

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


Thanks for your perspective, Mark. I appreciate your point of view.

@nicolasfella, sounds like you have the green light to adapt this patch to apply it to KIO, and we'll see how that turns out! I for one am really looking forward to it.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Michael Heidelbach]

michaelh added a comment.


I could not try this one. `arc patch D7700` failed against master and Applications/17.12.
This looks like a raw diff. If so, please consider using `arc`.
@ngraham: Are special techniques involved to `arc patch` a raw diff?
@spoorun: Probably Bug 388481 <https://bugs.kde.org/show_bug.cgi?id=388481>. Mind to add your observations to it?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Marijo Mustac]

mmustac added a comment.


I know this has nothing to do with this patch - maybe I should file a bug for this ? - anyway I wanted to share my thoughts after reading Mark's position and I think he's right. We should not put everything into the places panel nor should it be a sperate panel ... sound weird ? Well ... I like the idea of having different panels for different working scenarios but the static job they were created for is not ideal, altough we can hide groups from the panel now ... My idea is to be able to add one ore more panels which would be just a container which then can be filled with different views / models ? So for example I can create a panel by my own, give it a name and customize it do show a list of tags and shortcuts for my saved searches for example. Then another panel could contain my device list and the default places section and a third one to contain the directory tree. That way I would be able to switch very quick between them, don't need to scroll or reconfigure my panel over and over again. Maybe someone can follow me and what I am trying to describe. Dolphin then could offer some preconfigured panels by default but let the users all possibilities to customize it wahtever they like. Easy by default, powerfull when needed.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


I know this isn't the most technically elegant solution, but bikeshedding has already cost this patch 4 months and it now requires a total rebase on top of KIO. The more hurdles we put in Nico's path, the less likely it is to ever get implement in //any// form unless someone else wants to take it over.

That's an interesting idea, @mmustac, but as you said, it's somewhat unrelated to this particular patch. Maybe file a bugzilla wishlist ticket for it?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


@michaelheidelbach, applying the diff fails because the functionality this was built on top of has since been moved to KIO (which was really welcome work). This patch will need to be re-based (or more likely) re-written on top of KIO.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Henrik Fehlauer]

rkflx added a comment.

Post by Michael Heidelbach
I could not try this one. `arc patch D7700` failed against master and Applications/17.12.
This looks like a raw diff. If so, please consider using `arc`.

Phab shows a popup if you point your mouse over the date/time entry of a comment indicating a patch upload or update. Here we see "Via Web", but obviously `arc` would be more friendly for reviewers because of the included base revision.

---

Regarding the topic: I did not read everything in detail, but please make sure not to break Gwenview and other users of `KFilePlaces` again, I'm still busy with the fallout from the last breakage
 Also, performance matters, so ensure (and measure!) we won't regress for Dolphin's default config.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


@nicolasfella, could we resurrect this and re-base it on top of KIO? It's really great functionality, and I'd hate to see it lost to the sands of time.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.


I'll look into it the next days

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Michael Heidelbach]

michaelh added a comment.


@nicolasfella Beware `tags:/` currently is a little broken.

$ kioclient5 ls "tags:/Watched"
Log Horizon?/home/otto/Videos/Log Horizon

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Michael Heidelbach]

michaelh added a comment.


Citing myself

Just as an idea: Implement user defineable groups. That way users could drop their most important tags/searches/whatever into it and expand and collapse as they wish.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, isidorov, firef, andrebarros

[Nicolas Fella]

nicolasfella updated this revision to Diff 37300.
nicolasfella added a comment.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.


Add tags places in KIO

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=19502&id=37300

BRANCH
tags

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/filewidgets/kfileplacesitem.cpp
src/filewidgets/kfileplacesitem_p.h
src/filewidgets/kfileplacesmodel.cpp
src/filewidgets/kfileplacesmodel.h

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella updated this revision to Diff 38100.
nicolasfella added a comment.


Actually save tag bookmark and check if it already exists before adding

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=37300&id=38100

BRANCH
tags

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/filewidgets/kfileplacesitem.cpp
src/filewidgets/kfileplacesitem_p.h
src/filewidgets/kfileplacesmodel.cpp
src/filewidgets/kfileplacesmodel.h

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nathaniel Graham]

ngraham added a comment.


Results of testing:

- After I create a new tag, it doesn't show up in the Places panel tag list or under the `tags:/` ioslave until I restart Dolphin.
- When I navigate to `tags:/` ioslave using the All Tags item in the Places panel and I click on a tag, its icon disappears.
- When I remove a tag from an item and navigate to that tag entry on the Places panel, the un-tagged item still shows up there until I restart Dolphin.
- When I add a tag to an item and navigate to that tag entry on the Places panel, the item does not show up there until I restart Dolphin.

Some of these issues may be issues with the `tags:/` ioslave or in Baloo, but I think they need to be fixed before we push this feature front-and-center, or else we'll get a ton of bug reports.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, navarromorales, firef, andrebarros

[Nicolas Fella]

nicolasfella added a comment.

Post by Nathaniel Graham
- After I create a new tag, it doesn't show up in the Places panel tag list or under the `tags:/` ioslave until I restart Dolphin.
- When I navigate to `tags:/` ioslave using the All Tags item in the Places panel and I click on a tag, its icon disappears.
- When I remove a tag from an item and navigate to that tag entry on the Places panel, the un-tagged item still shows up there until I restart Dolphin.
- When I add a tag to an item and navigate to that tag entry on the Places panel, the item does not show up there until I restart Dolphin.
Some of these issues may be issues with the `tags:/` ioslave or in Baloo, but I think they need to be fixed before we push this feature front-and-center, or else we'll get a ton of bug reports.

I can't reproduce these issues, so I think that the underlying Baloo issues have been solved.
I suggest to reconsider this patch for inclusion, especially since it's non-invasive for users that don't make use of tags at all. If there are issues remaining it's easier to spot them when people are actually using it

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, alexde, sourabhboss, feverfew, navarromorales, firef, andrebarros, mikesomov

[Nathaniel Graham]

ngraham added a comment.


Me neither. Can you rebase this on current master?

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, alexde, sourabhboss, feverfew, navarromorales, firef, andrebarros, mikesomov

[Nicolas Fella]

nicolasfella updated this revision to Diff 45414.
nicolasfella added a comment.


- Merge branch 'master' into arcpatch-D7700

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=38100&id=45414

BRANCH
arcpatch-D7700

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/filewidgets/kfileplacesitem.cpp
src/filewidgets/kfileplacesitem_p.h
src/filewidgets/kfileplacesmodel.cpp
src/filewidgets/kfileplacesmodel.h

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, alexde, sourabhboss, feverfew, navarromorales, firef, andrebarros, mikesomov

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


A surprisingly small amount of code for a nice feature. Code looks good to me except for one minor thing. This works great in my testing now. I say let's ship it, especially because it won't result in any UI changes at all for people who don't use tags.

INLINE COMMENTS

kfileplacesmodel.cpp:187
+ while (!bookmark.isNull()) {
+ QUrl url = bookmark.url();
+

const. But in fact, does this even need to be a variable? It's only used once.

REPOSITORY
R241 KIO

BRANCH
arcpatch-D7700

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, alexde, sourabhboss, feverfew, navarromorales, firef, andrebarros, mikesomov

[Nathaniel Graham]

ngraham added a task: T8349: Improve Places panel usability and presentation.

REPOSITORY
R241 KIO

BRANCH
arcpatch-D7700

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, alexde, sourabhboss, feverfew, navarromorales, firef, andrebarros, mikesomov

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

ngraham wrote in kfileplacesmodel.cpp:187
const. But in fact, does this even need to be a variable? It's only used once.

Will fix on push

REPOSITORY
R241 KIO

BRANCH
arcpatch-D7700

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, alexde, sourabhboss, feverfew, navarromorales, firef, andrebarros, mikesomov

[Nicolas Fella]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:2ae8a0c374cd: Show list of tags in PlacesView (authored by nicolasfella).

CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D7700?vs=45414&id=45426#toc

REPOSITORY
R241 KIO

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7700?vs=45414&id=45426

REVISION DETAIL
https://phabricator.kde.org/D7700

AFFECTED FILES
src/filewidgets/kfileplacesitem.cpp
src/filewidgets/kfileplacesitem_p.h
src/filewidgets/kfileplacesmodel.cpp
src/filewidgets/kfileplacesmodel.h

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, alexde, sourabhboss, feverfew, navarromorales, firef, andrebarros, mikesomov

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

kfileplacesmodel.h:66
+ UnknownType,
+ TagsType
};

This is missing a `///< @since 5.53` comment.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7700

To: nicolasfella, #dolphin, #kde_applications, alexeymin, ngraham
Cc: kde-frameworks-devel, bruns, rkflx, mmustac, spoorun, michaelh, renatoo, anthonyfieroni, cfeck, elvisangelaccio, emmanuelp, ngraham, alexeymin, #dolphin, alexde, sourabhboss, feverfew, navarromorales, firef, andrebarros, mikesomov