This revision was automatically updated to reflect the committed changes.
Closed by commit R241:cd9856d14552: Hidding place groups implementation in
KFilePlacesModel (authored by franckarrecot).
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=23629&id=23632
franckarrecot updated this revision to Diff 23629.
franckarrecot added a comment.
rebase
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=23409&id=23629
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplacesmodeltest.
renatoo accepted this revision.
renatoo added a comment.
Looks good and works as expected
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
mwolff accepted this revision.
mwolff added a comment.
This revision is now accepted and ready to land.
lgtm
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
mlaurent accepted this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
franckarrecot updated this revision to Diff 23409.
franckarrecot added a comment.
rebase to master
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=23024&id=23409
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplaces
ngraham added a comment.
@mwolff, is this good now?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
ervin accepted this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
franckarrecot marked 15 inline comments as done.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
franckarrecot marked 2 inline comments as done.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
ngraham added a comment.
Can you mark the Not Done comments as Done if they're done now?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
franckarrecot updated this revision to Diff 23024.
franckarrecot added a comment.
update - should be landable now
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=22998&id=23024
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autote
franckarrecot added a dependent revision: D9015: Refactoring the
hidding/showing animation use within KFilePlacesView.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #framework
franckarrecot updated this revision to Diff 22998.
franckarrecot added a comment.
fix variable name
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=22901&id=22998
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplace
franckarrecot edited the summary of this revision.
franckarrecot added dependencies: D8948: Created an auxiliary function
'KFilePlacesModel::movePlace', D8947: Expose KFilePlacesModel 'iconName' role,
D8946: Avoid unnecessary 'dataChanged' signal, D8945: Return a valid bookmark
object for any e
ervin added inline comments.
INLINE COMMENTS
> franckarrecot wrote in kfileplacesmodel.cpp:870
> Because it wouldn't compile without the whole KFIlePlacesModel:: prefix, so I
> end up going for variable renaming, seemed cleaner
Let's go for naming it "groupHidden" then. It's a weird shortening
franckarrecot updated this revision to Diff 22901.
franckarrecot added a comment.
rebased over in review renato 's patches
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=22726&id=22901
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
franckarrecot marked 5 inline comments as done.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, ngraham, ervin, mwolff, mlaurent
Cc: mwolff, ngraham, mlaurent, #frameworks
franckarrecot added inline comments.
INLINE COMMENTS
> ervin wrote in kfileplacesmodel.cpp:870
> Why not naming the variable isGroupHidden and use the :: prefix on the
> function call like milian proposed?
Because it wouldn't compile without the whole KFIlePlacesModel:: prefix, so I
end up goi
ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.
A small nitpick otherwise lgtm.
INLINE COMMENTS
> kfileplacesmodel.cpp:870
>
> -bookmark.setMetaDataItem(QStringLiteral("IsHidden"), (hidden ?
> QStringLiteral("true") : QStr
franckarrecot added a comment.
precision on useless comment
INLINE COMMENTS
> franckarrecot wrote in kfileplacesmodel.cpp:257
> it is currently not needed here since each change an object metadata ( eg :
> item isHidden metadata ) would trigger a loadBookmarkList().
> And in this code we ma
franckarrecot updated this revision to Diff 22726.
franckarrecot edited the test plan for this revision.
franckarrecot added a comment.
update
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21755&id=22726
REVISION DETAIL
https://phabricator.kde.org/D
franckarrecot commandeered this revision.
franckarrecot edited reviewers, added: mlaurent; removed: franckarrecot.
franckarrecot added a comment.
taking ownership back to fix comments, thanks Laurent for taking care of it
:-)
INLINE COMMENTS
> renatoo wrote in kfileplacesmodel.cpp:257
> shou
mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kfileplacesmodel.cpp:70
> +{
> +{ KFilePlacesModel::PlacesType,
> QStringLiteral("GroupState-Places-IsHidden") },
> +{ KFilePlacesModel::R
ngraham added a dependent revision: D8450: User can now hide an entire places
group from KFilePlacesView.
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent, renatoo, ngraham, franckarrecot, ervin
Cc: ngraham, mlaurent, #frameworks
mlaurent updated this revision to Diff 21755.
mlaurent added a comment.
Remove commented code. Rename methods
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21752&id=21755
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplacesmodeltest.cp
mlaurent marked 2 inline comments as done.
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent, renatoo, ngraham, franckarrecot, ervin
Cc: ngraham, mlaurent, #frameworks
ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.
Couple more changes needed.
INLINE COMMENTS
> kfileplacesmodel.cpp:79-83
> +//static const QHash
> s_groupTypeToStateName
> +//{
> +//return *s_groupTypeToStateName;
> +//}
> +
mlaurent updated this revision to Diff 21752.
mlaurent added a comment.
Rebase against master + other top patchs :)
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21645&id=21752
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplacesmodelt
ngraham added a comment.
OK, great! Glad you're thinking about it and I'm happy we came to the same
conclusion. :) Later is fine.
This is awesome work, BTW.
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent, renatoo, ngraham, franckarrecot, ervin
Cc: ngraham, mlaurent, #f
renatoo added a comment.
In https://phabricator.kde.org/D8367#162465, @ngraham wrote:
> Nice! If possible, I'd like a more user-friendly way of hiding and showing
categories, though. The context menu is not very discoverable, and if a
category is hidden, there's no indication that there'
ngraham added a comment.
Nice! If possible, I'd like a more user-friendly way of hiding and showing
categories, though. The context menu is not very discoverable, and if a
category is hidden, there's no indication that there's even anything to show.
Here's how macOS finder handles this, for
mlaurent updated this revision to Diff 21645.
mlaurent added a comment.
Fixing create static variable
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21643&id=21645
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplacesmodeltest.cpp
src/
mlaurent marked an inline comment as done.
mlaurent added inline comments.
INLINE COMMENTS
> ervin wrote in kfileplacesmodel.h:55
> You missed a bit, it looks like it wasn't applied on top of the latest
> iteration from the other patches the GroupType enum gained some values for
> its entries.
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> mlaurent wrote in kfileplacesitem.cpp:202
> This one is for avoiding duplicate code as we create in this patch isHidden
> method.
That was kind of my point, is
mlaurent updated this revision to Diff 21643.
mlaurent added a comment.
updated according to comments
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21605&id=21643
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplacesmodeltest.cpp
src/
mlaurent marked 6 inline comments as done.
mlaurent added inline comments.
INLINE COMMENTS
> ervin wrote in kfileplacesitem.cpp:202
> Very welcome refactoring... in a separate commit
This one is for avoiding duplicate code as we create in this patch isHidden
method.
REVISION DETAIL
https://p
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kfileplacesmodeltest.cpp:504
> QList args;
> -QSignalSpy spy_inserted(m_places,
> SIGNAL(rowsInserted(QModelIndex,int,int)));
> -QSignalSpy spy_rem
mlaurent edited the test plan for this revision.
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent, renatoo, ngraham, franckarrecot, ervin
Cc: ngraham, mlaurent, #frameworks
mlaurent edited the test plan for this revision.
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent, renatoo, ngraham, franckarrecot, ervin
Cc: ngraham, mlaurent, #frameworks
mlaurent updated this revision to Diff 21605.
mlaurent added a comment.
Remove duplicate code
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21604&id=21605
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplacesmodeltest.cpp
src/filewidg
mlaurent edited the summary of this revision.
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent, renatoo, ngraham, franckarrecot
Cc: ngraham, mlaurent, #frameworks
mlaurent added a reviewer: ervin.
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent, renatoo, ngraham, franckarrecot, ervin
Cc: ngraham, mlaurent, #frameworks
mlaurent updated this revision to Diff 21604.
mlaurent added a comment.
Modernize code.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21572&id=21604
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplacesmodeltest.cpp
src/filewidgets/kf
mlaurent edited the summary of this revision.
mlaurent added dependencies: D8366: Factoring out lists of url data within
KFilePlacesModelTest, D8348: Add a section for removable devices, D8332: Added
baloo urls into places model.
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent,
mlaurent updated this revision to Diff 21572.
mlaurent added a comment.
We can't use qAsConst as it's a qt5.8 macro.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21290&id=21572
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotests/kfileplacesmodel
mlaurent commandeered this revision.
mlaurent edited reviewers, added: franckarrecot; removed: mlaurent.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: mlaurent, renatoo, ngraham, franckarrecot
Cc: ngraham, mlaurent, #frameworks
mlaurent added a comment.
depend against https://phabricator.kde.org/D8366 too...
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, mlaurent, ngraham
Cc: ngraham, mlaurent, #frameworks
mlaurent added a comment.
it depends against https://phabricator.kde.org/D8348 too
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, mlaurent, ngraham
Cc: ngraham, mlaurent, #frameworks
mlaurent added a comment.
it depends against https://phabricator.kde.org/D8332.diff too
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, mlaurent, ngraham
Cc: ngraham, mlaurent, #frameworks
franckarrecot updated this revision to Diff 21290.
franckarrecot added a comment.
updated according to comments
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21243&id=21290
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECTED FILES
autotest
mlaurent added inline comments.
INLINE COMMENTS
> kfileplacesmodel.cpp:596
> +const QHash groupStates = groupStateHidden(root);
> +for (KFilePlacesItem *item : items) {
> +if (bool hasToBeHidden = groupStates.value(item->groupType())) {
for(... : qAsConst(items)) {
}
REPOSITORY
renatoo added a comment.
Pleas unit test "hiddenCount" when groups are invisible, it is used by the
view in https://phabricator.kde.org/D8450
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8367
To: franckarrecot, renatoo, mlaurent, ngraham
Cc: ngraham, mlaurent, #frame
renatoo added inline comments.
INLINE COMMENTS
> kfileplacesmodel.cpp:257
>
> bool KFilePlacesModel::isHidden(const QModelIndex &index) const
> {
should this check if the group which the index belongs is hider or not?
Something like:
return (data(index, HiddenRole).toBool() || isPlaceGro
franckarrecot updated this revision to Diff 21243.
franckarrecot added a comment.
adding more services from model usefull to views
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21177&id=21243
REVISION DETAIL
https://phabricator.kde.org/D8367
AFFECT
franckarrecot updated this revision to Diff 21177.
franckarrecot added a comment.
Moving enum from item to model to avoid exposing private api
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21167&id=21177
REVISION DETAIL
https://phabricator.kde.org/D
franckarrecot updated this revision to Diff 21167.
franckarrecot edited the summary of this revision.
franckarrecot added a comment.
sync summary
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8367?vs=21166&id=21167
REVISION DETAIL
https://phabricator.kde.or
franckarrecot retitled this revision from "WIP: Hidding Group from
KFilePlacesModel" to "Hidding place groups implementation in KFilePlacesModel".
franckarrecot edited the summary of this revision.
franckarrecot edited the test plan for this revision.
franckarrecot added a reviewer: ngraham.
franc
58 matches
Mail list logo
