Implemented support for hide/show groups
ClosedPublic
Actions

Authored by renatoo on Dec 7 2017, 2:38 PM.

Details

Reviewers

franckarrecot
mlaurent
mwolff
elvisangelaccio

Commits

R318:5f1df43b8789: Implemented support for hide/show groups

Summary

Added an option on PlacesPanel context menu to show or hide the entire
group of places.

Depends on D8855

Test Plan

Open Donlphin and use PlacesPanel context menu to hide and show groups

Diff Detail

Repository

R318 Dolphin

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

renatoo created this revision.Dec 7 2017, 2:38 PM

Restricted Application added a subscriber: Dolphin. · View Herald TranscriptDec 7 2017, 2:38 PM

renatoo requested review of this revision.Dec 7 2017, 2:38 PM

ngraham added a subscriber: ngraham.Dec 7 2017, 2:38 PM

renatoo edited the summary of this revision. (Show Details)Dec 7 2017, 2:38 PM

renatoo added a dependency: D8855: Use Kio::KPlacesModel as source model for PlacesItemModel.

renatoo added a reviewer: franckarrecot.Dec 7 2017, 2:53 PM

mlaurent accepted this revision.Dec 8 2017, 8:49 AM

This revision is now accepted and ready to land.Dec 8 2017, 8:49 AM

elvisangelaccio added a subscriber: elvisangelaccio.Dec 8 2017, 9:10 AM

elvisangelaccio added inline comments.

src/panels/places/placesitemmodel.h
24	#include <KFilePlacesModel> ?

I agree with the suggestion by Elvis, otherwise lgtm besides one more suggestion for potential cleanup

src/panels/places/placesitemmodel.cpp
477	usually it would be preferred if you could add a proper role for the group type. Then you could just do auto groupType = index.data(KFilePlacesModel::GroupTypeRole).value<KFilePlacesModel::GroupType>(); item->setGroupHidden(model->isGrouHidden(groupType)); at this point, it's then even better to just make this possible: auto groupHidden = index.data(KFilePlacesModel::GroupHiddenRole).toBool(); item->setGroupHidden(groupHidden);

src/panels/places/placesitemmodel.cpp

477

usually it would be preferred if you could add a proper role for the group type. Then you could just do

auto groupType = index.data(KFilePlacesModel::GroupTypeRole).value<KFilePlacesModel::GroupType>();
item->setGroupHidden(model->isGrouHidden(groupType));

at this point, it's then even better to just make this possible:

auto groupHidden = index.data(KFilePlacesModel::GroupHiddenRole).toBool();
item->setGroupHidden(groupHidden);

renatoo edited the summary of this revision. (Show Details)Dec 8 2017, 1:21 PM

renatoo added a dependency: D9252: Created 'GroupHiddenRole' for KPlacesModel.

Used new KFilePlacesModel::GroupHiddenRole to retrieve information instead of call the model method

@elvisangelaccio ist that ok now?

I can't apply this patch, can you please rebase it?

Rebased with master

In D9242#177648, @elvisangelaccio wrote:

I can't apply this patch, can you please rebase it?

done

In D9242#177654, @renatoo wrote:

In D9242#177648, @elvisangelaccio wrote:

I can't apply this patch, can you please rebase it?

done

arc patch D9242 still doesn't work, looks like something else is missing.

It appears we still need to land D8855 first.

In D9242#177703, @elvisangelaccio wrote:

In D9242#177654, @renatoo wrote:

In D9242#177648, @elvisangelaccio wrote:

I can't apply this patch, can you please rebase it?

done

arc patch D9242 still doesn't work, looks like something else is missing.

works nice for me

In D9242#177720, @ngraham wrote:

It appears we still need to land D8855 first.

I do not think this is the problem since that after run: arc patch D9242. I got both changes on my tree.

In D9242#177723, @renatoo wrote:

In D9242#177720, @ngraham wrote:

It appears we still need to land D8855 first.

I do not think this is the problem since that after run: arc patch D9242. I got both changes on my tree.

I get:

$ arc patch D9242
 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D9242.
 INFO  Base commit is not in local repository; trying to fetch.
Checking patch src/filewidgets/kfileplacesmodel.h...
error: src/filewidgets/kfileplacesmodel.h: does not exist in index
Checking patch src/filewidgets/kfileplacesmodel.cpp...
error: src/filewidgets/kfileplacesmodel.cpp: does not exist in index
Checking patch autotests/kfileplacesmodeltest.cpp...
error: autotests/kfileplacesmodeltest.cpp: does not exist in index

 Patch Failed! 
Usage Exception: Unable to apply patch!

In D9242#177724, @elvisangelaccio wrote:

In D9242#177723, @renatoo wrote:

In D9242#177720, @ngraham wrote:

It appears we still need to land D8855 first.

I do not think this is the problem since that after run: arc patch D9242. I got both changes on my tree.

I get:

$ arc patch D9242
 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D9242.
 INFO  Base commit is not in local repository; trying to fetch.
Checking patch src/filewidgets/kfileplacesmodel.h...
error: src/filewidgets/kfileplacesmodel.h: does not exist in index
Checking patch src/filewidgets/kfileplacesmodel.cpp...
error: src/filewidgets/kfileplacesmodel.cpp: does not exist in index
Checking patch autotests/kfileplacesmodeltest.cpp...
error: autotests/kfileplacesmodeltest.cpp: does not exist in index

 Patch Failed! 
Usage Exception: Unable to apply patch!

very strange why this patch is checking for KIO files? This is a dolphin patch

rkflx added a subscriber: rkflx.Dec 8 2017, 8:42 PM

In D9242#177725, @renatoo wrote:

very strange why this patch is checking for KIO files? This is a dolphin patch

The summary has a dependency on D9252, which makes arc patch fetch it. However, the dependency itself is based on a git commit hash from R241 KIO and does not relate to the current git repo, so things go wrong. Try removing the dependency from the summary and then test with a clean repo checkout.

Alternatively, try arc patch --skip-dependencies.

In D9242#177648, @elvisangelaccio wrote:

I can't apply this patch, can you please rebase it?

@elvisangelaccio I removed the KIO dependency from the summary could you try again? Or use the solution proposed by @rkflx

@elvisangelaccio could you try again?

This time I got: https://paste.kde.org/p02sygoma

(I tried on fresh clone)

In D9242#178869, @elvisangelaccio wrote:

This time I got: https://paste.kde.org/p02sygoma

(I tried on fresh clone)

And if I manually download the patch, it fails with a similar error when applying it.

If you use --skip-dependencies, you'd have to apply D8855 manually too, of course. Then it works for me.

The reason arc patch D9242 still does not work is because the dependency on D9252 is still there. Just removing it from the summary is not enough apparently, you have to use "Edit Related Revisions". It is all very logical once you stumbled upon this the first time…

Try this, I hope then it will work. Also, look at the "Stack" tab to see what's going on…

elvisangelaccio removed a dependency: D9252: Created 'GroupHiddenRole' for KPlacesModel.Dec 12 2017, 9:20 PM

For the record:

arc patch D8855
arc patch D9242 --skip-dependencies

this worked for me. Building now...

Patch seems to work fine, nice job!

src/panels/places/placespanel.cpp
340	Should we add this new action/option in dolphin_placespanelsettings.kcfg ?

(OT)

Interesting that plain arc patch still fails. --trace shows it tries to fetch 7ef5e26f2f474f3245dedfbb4aa2e5c1acceb951 from R241 KIO, which is the parent commit of D8243, i.e. a transitive dependency of D8855. The error message matches exactly the files from D8243, too.

What I cannot figure out is why applying D8855 works, and for D9242 it fails. Sounds like a bug in arc, of which there are plenty in upstream Phab. In general cross-repo dependencies are not a sensible thing to have, so perhaps cleaning this up (i.e. fixing the dependencies of D8855) would help. However, I will stop here as this is not worth the effort ;)

renatoo added inline comments.Dec 13 2017, 12:02 PM

src/panels/places/placespanel.cpp
340	I do not think that is necessary we already store the visible/hide group on the bookmark file, storing it in another file will require a new level of abstraction to filter it and make it even more complex.

franckarrecot accepted this revision.Dec 13 2017, 12:25 PM

LGTM then.

Closed by commit R318:5f1df43b8789: Implemented support for hide/show groups (authored by Renato Araujo Oliveira Filho <renato.araujo@kdab.com>). · Explain WhyDec 14 2017, 12:42 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			src/kitemviews/kitemlistcontroller.h (2 lines)
M			src/kitemviews/kitemlistcontroller.cpp (14 lines)
M			src/panels/places/placesitem.h (3 lines)
M			src/panels/places/placesitem.cpp (10 lines)
M			src/panels/places/placesitemlistwidget.cpp (3 lines)
M			src/panels/places/placesitemmodel.h (9 lines)
M			src/panels/places/placesitemmodel.cpp (35 lines)
M			src/panels/places/placespanel.h (3 lines)
M			src/panels/places/placespanel.cpp (22 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	23615	7756326		Dec 7 2017, 2:38 PM	★	★
Diff 2	23646	7756326	Used new KFilePlacesModel::GroupHiddenRole to retrieve information instead of…	Dec 8 2017, 1:26 PM	★	★
Diff 3	23663	ad8dd2f	Rebased with master	Dec 8 2017, 5:05 PM	★	★
Diff 4	23903	da6f8fe	R318:5f1df43b87898b380228a3548553de3290ddb0d7	Dec 14 2017, 12:42 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
39e56e9e48f8	535653ac2eb2	ad8dd2f61b8f	Renato Araujo Oliveira Filho	Implemented support for hide/show groups (Show More…)	Dec 7 2017, 2:36 PM

Status	Author	Revision
Closed	mlaurent	D8619 Refactor and remove duplicate code in kfileplacesview
Closed	renatoo	D9241 Emit 'groupHiddenChanged' signal.
Closed	franckarrecot	D9015 Refactoring the hidding/showing animation use within KFilePlacesView
Closed	franckarrecot	D8450 User can now hide an entire places group from KFilePlacesView
Closed	franckarrecot	D8367 Hidding place groups implementation in KFilePlacesModel
Closed	franckarrecot	D8366 Factoring out lists of url data within KFilePlacesModelTest
Closed	renatoo	D8948 Created an auxiliary function 'KFilePlacesModel::movePlace'
Closed	renatoo	D8947 Expose KFilePlacesModel 'iconName' role
Closed	renatoo	D8946 Avoid unnecessary 'dataChanged' signal
Closed	renatoo	D8945 Return a valid bookmark object for any entry in KFilePlacesModel
Closed	renatoo	D8944 Create a 'KFilePlacesModel::refresh' function
Closed	renatoo	D8943 Create 'KFilePlacesModel::convertedUrl' static function
Closed	renatoo	D9242 Implemented support for hide/show groups
Abandoned	ngraham	D8855 Use Kio::KPlacesModel as source model for PlacesItemModel
Abandoned	renatoo	D8862 Extend KFilePlacesModel API
Closed	renatoo	D8630 Created unit test for PlacesItemModel
Closed	renatoo	D8434 Created 'remote' section
Closed	renatoo	D8348 Add a section for removable devices
Closed	renatoo	D8332 Added baloo urls into places model
Closed	renatoo	D8243 Implement support for categories on KfilesPlacesView