D8243: Implement support for categories on KfilesPlacesView
renatoo added a comment. In https://phabricator.kde.org/D8243#173442, @broulik wrote: > Can we expose the group type enum in the model in addition to the name, so I can do some filtering in e.g. task manager? This will be exposed by this mr: https://phabricator.kde.org/D8367#change-GqgZlwb2MzB8 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg, ngraham Cc: broulik, markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
broulik added a comment. Can we expose the group type enum in the model in addition to the name, so I can do some filtering in e.g. task manager? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg, ngraham Cc: broulik, markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
This revision was automatically updated to reflect the committed changes. Closed by commit R241:7a6eff3fce41: Implement support for categories on KfilesPlacesView (authored by Renato Araujo Oliveira Filho renato.ara...@kdab.com, committed by ngraham). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=21582=21636 REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin, #vdg, ngraham Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ngraham accepted this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg, ngraham Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ervin added a comment. Great work BTW, this looks really nice now. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ervin accepted this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 21582. renatoo added a comment. Updated visuals to match dolphin REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=21226=21582 REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo marked 3 inline comments as done. renatoo added inline comments. INLINE COMMENTS > ervin wrote in kfileplacesmodel.cpp:476 > Either reorder the enum or change for a different (more explicit not relying > on enum storage) comparison operator to have the "right" order. By right > order I assume the goal was to align with Dolphin which does: Places / > Recently Saved / Search For / Devices. > > Currently we have a different order: Places / Search For / Recently Saved / > Devices. Fixed enum order and set values for it. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo added inline comments.
INLINE COMMENTS
> ervin wrote in kfileplacesmodel.cpp:476
> Either reorder the enum or change for a different (more explicit not relying
> on enum storage) comparison operator to have the "right" order. By right
> order I assume the goal was to align with Dolphin which does: Places /
> Recently Saved / Search For / Devices.
>
> Currently we have a different order: Places / Search For / Recently Saved /
> Devices.
Are you fine If I set values for enums? Something like:
enum GroupType
{
PlacesType = 100,
RecentlySavedType = 200,
SearchForType = 300,
DevicesType = 400,
RemovableDeviceType = 500,
NetworkType = 600
};
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure, ervin, #vdg
Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ervin added a comment.
Looks fine code wise now, just a couple more tweaks to make those sections
look closer to what Dolphin got.
INLINE COMMENTS
> kfileplacesmodel.cpp:476
> +[](KFilePlacesItem *itemA, KFilePlacesItem *itemB) {
> + return (itemA->groupType() < itemB->groupType());
> +});
Either reorder the enum or change for a different (more explicit not relying on
enum storage) comparison operator to have the "right" order. By right order I
assume the goal was to align with Dolphin which does: Places / Recently Saved /
Search For / Devices.
Currently we have a different order: Places / Search For / Recently Saved /
Devices.
> kfileplacesview.cpp:458
> +{
> +return QApplication::fontMetrics().height();
> +}
With again the goal to get closer to Dolphin's rendering, its section headers
are higher. The text size you use is correct during drawing but they happen to
have much more white space before and a bit more after giving it some more
breath.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure, ervin, #vdg
Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo added a comment. Guys, is that ready? Do you need any other change/fix? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 21226. renatoo added a comment. Fixed typo REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20951=21226 REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
dfaure added inline comments. INLINE COMMENTS > kfileplacesview.cpp:1146 > + > +const int totlaItemsHeight = (fm.height() / 2) * rowCount; > +const int totoalSectionsHeight = delegate->sectionHeaderHeight() * > sectionsCount(); typo (total) > kfileplacesview.cpp:1147 > +const int totlaItemsHeight = (fm.height() / 2) * rowCount; > +const int totoalSectionsHeight = delegate->sectionHeaderHeight() * > sectionsCount(); > +const int maxHeight = ((q->height() - totoalSectionsHeight - > totlaItemsHeight) / rowCount) - 1; different typo (total) ;-) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ngraham added a comment. @dfaure and @ervin, is this acceptable now? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo marked 3 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
franckarrecot added a dependent revision: D8367: Hidding place groups implementation in KFilePlacesModel. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ngraham added a comment. @renatoo, there are still a few Not Done comments. Any chance you can resolve them so we can push this forward? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ngraham added a comment. @markg Ultimately I think everyone is in agreement in wanting the "single scrollbar" approach for items in whatever you call the default list on Dolphin's left side. Splitting everything into distinct panels and embedding all the panels in a single scrollview is one way to get there. Putting all the features people want in the Places panel is another way. I don't have super strong feelings on the approach taken, because I think either one will get where we want to go, but I do see a lot more activity and excitement in the "add things to the Places panel" approach. Since you prefer the "make everything its own panel" approach, maybe you can submit a patch so we can judge the different approaches somewhere separate and we don't have to have the same discussion every time someone proposes adding something to the places panel. :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
markg added a comment. In https://phabricator.kde.org/D8243#157346, @renatoo wrote: > In https://phabricator.kde.org/D8243#157336, @markg wrote: > > > We have such a nice modular design with panels yet we keep cramming everything in the places panel... Why? > > It's not just here, I've seen patches like this for Dolphin as well. > > > > Imho, the places panel should only contain the top places - favorites if you will - that you might want to access quickly. Nothing else and should stay small. > > Baloo -> new panel > > Devices -> new panel > > Remote devices -> Either in Devices or a new panel as well > > etc.. > > > > Having more panels will give a whole different issues. Scrolling. Since each panel would be relatively small if you add many you will get scrollbars within the panels. Well, lets just get rid of the per panel scrollbar and merely have one for the whole "panel container" (like in macOS finder). > > > > Just my 5 cents... > > > @markg, I never used macOs finder, but looking at the internet images. This is exactly what I am proposing and what we have on dolphin today. A single list with only one scrollbar split in sections. > > I am just trying to keep the same visuals on both (dolphin and file dialog) to avoid confusion for users, they can get lost if you show different information. > > > Our final idea is that you will be able to hide sections that you do not want, For example the local devices and network. You "intend" to, yes :) But with your changes you get the situation where you have multiple scrollbars if you have multiple panels enabled, one scrollbar in each panel. In Finder, you have 1 scrollbar for the whole panel area. I like the goal! I want to get there as well! But it would be my preference to get there with individual panels. That would also make the code much simpler. Anyhow, that's just a "i would like..." opinion. It's not a critic to slow you down or stop this patch. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo added a comment. In https://phabricator.kde.org/D8243#157336, @markg wrote: > We have such a nice modular design with panels yet we keep cramming everything in the places panel... Why? > It's not just here, I've seen patches like this for Dolphin as well. > > Imho, the places panel should only contain the top places - favorites if you will - that you might want to access quickly. Nothing else and should stay small. > Baloo -> new panel > Devices -> new panel > Remote devices -> Either in Devices or a new panel as well > etc.. > > Having more panels will give a whole different issues. Scrolling. Since each panel would be relatively small if you add many you will get scrollbars within the panels. Well, lets just get rid of the per panel scrollbar and merely have one for the whole "panel container" (like in macOS finder). > > Just my 5 cents... @markg, I never used macOs finder, but looking at the internet images. This is exactly what I am proposing and what we have on dolphin today. A single list with only one scrollbar split in sections. I am just trying to keep the same visuals on both (dolphin and file dialog) to avoid confusion for users, they can get lost if you show different information. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
markg added a comment. We have such a nice modular design with panels yet we keep cramming everything in the places panel... Why? It's not just here, I've seen patches like this for Dolphin as well. Imho, the places panel should only contain the top places - favorites if you will - that you might want to access quickly. Nothing else and should stay small. Baloo -> new panel Devices -> new panel Remote devices -> Either in Devices or a new panel as well etc.. Having more panels will give a whole different issues. Scrolling. Since each panel would be relatively small if you add many you will get scrollbars within the panels. Well, lets just get rid of the per panel scrollbar and merely have one for the whole "panel container" (like in macOS finder). Just my 5 cents... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ngraham added a comment. @renatoo, can you update any remaining Not Done inline comments that are done now (including the discussion ones)? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20951. renatoo added a comment. Fixed places item size calculation REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20939=20951 REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
dfaure added a comment. +1, looks good to me, implementation wise. I'll let someone else decide on the actual feature (e.g. retesting for bugs, like Kévin did). I suspect that the use of QApplication::font/fontMetrics breaks when setting a font on that widget, but well, who would do that ;-) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20939. renatoo marked 11 inline comments as done. renatoo edited the summary of this revision. renatoo added a comment. Fixed typo and code style REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20675=20939 REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kfileplacesmodeltest.cpp:323
> +// Trying move the remote at the end of the list, should move it to the
> end of places section instead
> +// too keep it grouped
> KBookmark last = root.first();
too -> to
> kfileplacesitem.cpp:127
> +default:
> +Q_ASSERT(false);
> +break;
Q_UNREACHABLE() exists for this purpose
> kfileplacesview.cpp:108
> +
> +mutable bool m_dragStarted;
> +
move this bool next to the other one (m_showHoverIndication), to save on
padding.
> kfileplacesview.cpp:110
> +
> +QString groupNameFromIndex(const QModelIndex ) const;
> +QModelIndex previousVisibleIndex(const QModelIndex ) const;
Please move all these methods before the member variables.
This file was doing it correctly, with member vars at the end of the class, I'd
like this to be kept.
Otherwise it becomes hard to see all member vars, when they are in the middle
of methods.
> kfileplacesview.cpp:168
> +if (indexIsSectionHeader(index)) {
> +// if is drawing the floating element used by drag/drop, does not
> draw the header
> +if (!m_dragStarted) {
The grammar is a bit strange.
I'd say
"When drawing"
or
"If we are drawing"
and later on "do not draw".
> kfileplacesview.cpp:178
> +
> +if (m_dragStarted) {
> +m_dragStarted = false;
this if() doesn't seem very useful to me.
> kfileplacesview.cpp:354
> +{
> +// we only accept drag events starting from item boddy, ignore drag
> request from header
> +QModelIndex index = m_view->indexAt(pos);
boddy -> body
> kfileplacesview.cpp:461
> +{
> +QFontMetrics fm(QApplication::font());
> +return fm.height();
Use QApplication::fontMetrics() instead, it'll be much faster.
> kfileplacesview.cpp:737
>
> -if (index.isValid()) {
> +const bool clickOverItemArea =
> !delegate->pointIsHeaderArea(event->pos());
> +
what if we click into an empty space? then it's neither header nor item, right?
If I'm right then the naming here is confusing, at least, and should be
const bool clickOverHeader = delegate->pointIsHeaderArea(event->pos());
if (!clickOverHeader && )
> kfileplacesview.cpp:1258
> +
> +for(int i = 0; i < q->model()->rowCount(); i++) {
> +if (!q->isRowHidden(i)) {
Move rowCount to local variable. This is a non-inline virtual method, no way
the compiler can know that it's constant.
> kfileplacesview.cpp:1260
> +if (!q->isRowHidden(i)) {
> +QModelIndex index = q->model()->index(i, 0);
> +const QString sectionName =
> index.data(KFilePlacesModel::GroupRole).toString();
const
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure, ervin, #vdg
Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ngraham added a reviewer: VDG. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin, #vdg Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo added a dependent revision: D8332: Added baloo urls into places model. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo added a dependent revision: D8348: Add a section for removable devices. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ngraham added a comment. FWIW, I don't have a lot to offer on the code itself, but I strongly support the change itself. It's always bothered me that Open/Save dialogs were lacking these very useful sections REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ngraham added a comment. Screenshots, my man! Screenshots! Add them to the Summary section so people can see what they're getting if this gets merged. :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20675. renatoo added a comment. Updated unit test REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20674=20675 BRANCH model-hide-row REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20674. renatoo marked 8 inline comments as done. renatoo edited the summary of this revision. renatoo added a comment. updated comit summary renamed m_dragModeCount to m_dragStart refactory 'KFilePlacesItem::data' function REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20656=20674 BRANCH model-hide-row REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ervin added a comment.
A couple more changes needed.
INLINE COMMENTS
> kfileplacesitem.cpp:153-158
> +if (role == KFilePlacesModel::GroupRole) {
> +return QVariant(m_groupName);
> +}
>
> +QVariant returnData;
> if (role != KFilePlacesModel::HiddenRole && role != Qt::BackgroundRole
> && isDevice()) {
Nitpicking a bit (and sorry for not realizing it earlier) but I think I'd try
to keep the structure of the method before that patch and so go for:
if (role == KFilePlacesModel::GroupRole)
...
else if (role != KFilePlacesModel::HiddenRole ...)
...
else
Probably returnData can go away though and just return from the branches of the
if instead.
> kfileplacesview.cpp:108
> +
> +mutable int m_dragModeCount;
> +
OK, for that one you can probably get away with a bool instead (let's call it
m_dragStarted maybe?). The counter was for a more generic case of more than one
item being draggable at the same time, but that won't be the case here since
it's a single selection list.
> anthonyfieroni wrote in kfileplacesview.cpp:169
> I think in to move away from workarounds. Cause we want to draw header only
> ones i you ask to check a QStyleOptionViewItem index if you can use it
> insetad of dragcount.
@anthonyfieroni Well, at paint time you need to determine if a drag is ongoing
or not. There's no way the index can tell you that.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure, ervin
Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ervin requested changes to this revision. ervin added a comment. This revision now requires changes to proceed. You probably missed my comment on the previous iteration: "Note that the summary part of the commit is now wrong (it still refers to KCategorizedView). Also, KFilePlacesModelTest needs to be adjusted to take into account the new behavior." Please address those too. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
anthonyfieroni added inline comments. INLINE COMMENTS > renatoo wrote in kfileplacesview.cpp:169 > Humm I did not know about this opt.index property. Did some debug here and > it is always -1 . > > Sorry but I am not sure why should I care about opt.index? Checking the > "index" arg is not enough? > > I am checking it with "indexIsSectionHeader" which checks if we should draw > the section header or not. (it will return "true" for index 0) I think in to move away from workarounds. Cause we want to draw header only ones i you ask to check a QStyleOptionViewItem index if you can use it insetad of dragcount. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kfileplacesview.cpp:169 > Ok i understaind the workaround, but did you know what is the value of > opt.index.row() ? Humm I did not know about this opt.index property. Did some debug here and it is always -1 . Sorry but I am not sure why should I care about opt.index? Checking the "index" arg is not enough? I am checking it with "indexIsSectionHeader" which checks if we should draw the section header or not. (it will return "true" for index 0) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
anthonyfieroni added inline comments. INLINE COMMENTS > renatoo wrote in kfileplacesview.cpp:169 > m_dragModeCount is used as workaround to draw items on floating window during > the drag. > > When drag starts it save the number of selected items and until we draw all > selected items once we do not draw the section header (we do not want this to > appear on the dragging item), because this paint event will be used to > represent the floating item. > > Based on: https://github.com/KDE/zanshin/commit/ce6a5120 Ok i understaind the workaround, but did you know what is the value of opt.index.row() ? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo marked an inline comment as done. renatoo added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kfileplacesview.cpp:169 > Why you are sure that m_dragModeCount is 0 when indexIsSectionHeader? Why not > when index.row() == 0 we can paint section header? m_dragModeCount is used as workaround to draw items on floating window during the drag. When drag starts it save the number of selected items and until we draw all selected items once we do not draw the section header (we do not want this to appear on the dragging item), because this paint event will be used to represent the floating item. Based on: https://github.com/KDE/zanshin/commit/ce6a5120 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
anthonyfieroni added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kfileplacesview.cpp:1150 > This calculation makes no sense to me. We have ((x - (y/2) * rowCount) / > rowCount so rowCount can be removed no? Now i see i'm in wrong :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
anthonyfieroni added inline comments.
INLINE COMMENTS
> kfileplacesview.cpp:169
> +// if is drawing the floating element used by drag/drop, does not
> draw the header
> +if (m_dragModeCount == 0) {
> +drawSectionHeader(painter, opt, index);
Why you are sure that m_dragModeCount is 0 when indexIsSectionHeader? Why not
when index.row() == 0 we can paint section header?
> kfileplacesview.cpp:1150
> const int maxWidth = q->viewport()->width() - textWidth - 4 * margin - 1;
> -const int maxHeight = ((q->height() - (fm.height() / 2) * rowCount) /
> rowCount) - 1;
> +int maxHeight = ((q->height() - (fm.height() / 2) * rowCount) /
> rowCount) - 1;
> +maxHeight += sectionsCount() * fm.height();
This calculation makes no sense to me. We have ((x - (y/2) * rowCount) /
rowCount so rowCount can be removed no?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure, ervin
Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo marked 2 inline comments as done. renatoo added inline comments. INLINE COMMENTS > renatoo wrote in kfileplacesview.cpp:424 > I need a way query the header size height from "pointIsHeaderArea" to know if > the user clicked on header or in the real item. Since I do not have access > from "QStyleOptionViewItem" from there, I need to store it in a var to use it > late if necessary. > > I do not love the way that is implemented but, I can not find a better way to > implement that. Ok instead of use QStyleOptionViewItem information to check the font size of the header. Now I use QApplication::font, this give me flexibility to check the header size at any part of the code. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20656. renatoo added a comment. Use QApplication::font() to determine the section header font size REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20650=20656 BRANCH model-hide-row REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kfileplacesview.cpp:424 > So you mean sizeHint have a right height ? This height store looks like a > workaround. I need a way query the header size height from "pointIsHeaderArea" to know if the user clicked on header or in the real item. Since I do not have access from "QStyleOptionViewItem" from there, I need to store it in a var to use it late if necessary. I do not love the way that is implemented but, I can not find a better way to implement that. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
anthonyfieroni added inline comments.
INLINE COMMENTS
> kfileplacesview.cpp:424
> +if (m_sectionHeaderHeight == -1) {
> +m_sectionHeaderHeight = option.fontMetrics.height();
> +}
So you mean sizeHint have a right height ? This height store looks like a
workaround.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure, ervin
Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20650. renatoo marked an inline comment as done. renatoo added a comment. Optimize sectionsCount to not use a QSet Use static_cast when possible REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20646=20650 BRANCH model-hide-row REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kfileplacesview.cpp:997
> +{
> +KFilePlacesViewDelegate *delegate = dynamic_cast *>(itemDelegate());
> +
If you are sure that the delegate is a KFilePlacesViewDelegate, then use
static_cast.
If you aren't sure, then test for nullptr.
An unchecked dynamic_cast makes no sense.
> kfileplacesview.cpp:1253
> +{
> +QSet sections;
> +
Missing
sections.reserve(q->model()->rowCount())
(well, then better put the rowCount into a local variable, anyway).
Overall, filling a set just to count occurences seems like much work for just
one number in the end, but I don't know if there's a faster algorithm that can
be used. Would I be correct if I said that those GroupRole values are sorted?
Couting unique values in a list like {A, A, B, B, B, C, C} doesn't require a
full QSet to be filled in, one can just increment the count when the current
value differs from the previous one.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure, ervin
Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20646. renatoo added a comment. Fixed code style REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20645=20646 BRANCH model-hide-row REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo added a comment. All problems fixed. INLINE COMMENTS > anthonyfieroni wrote in kfileplacesview.cpp:1065-1066 > So if second line is correct why not remove first one? Sorry wrong operator it should be "+=" REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20645. renatoo marked 9 inline comments as done. renatoo added a comment. Fix code syntax Disable drag from item header Does not show item header on floating preview during the drag REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20613=20645 BRANCH model-hide-row REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
mlaurent added inline comments. INLINE COMMENTS > kfileplacesview.cpp:113 > +QColor baseColor(const QStyleOption ) const; > +QColor mixedColor(const QColor& c1, const QColor& c2, int c1Percent) > const; > }; coding style const QColor , const QColor REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ervin added a comment. One more thing, the way the sections are currently done, if I drag the first item of a section the visual feedback I get also includes the section itself giving the feeling that the section itself is being moved. Also I can drag from the section title itself which is a problem too. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure, ervin Cc: ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.
Note that the summary part of the commit is now wrong (it still refers to
KCategorizedView).
Also, KFilePlacesModelTest needs to be adjusted to take into account the new
behavior.
INLINE COMMENTS
> kfileplacesitem.cpp:179
> +
> +if (protocol == QLatin1String("bluetooth") || protocol ==
> QLatin1String("obexftp") || protocol == QLatin1String("kdeconnect")) {
> +return DevicesType;
Long line, would be nice to split it.
> kfileplacesitem_p.h:106
> QStringList m_emblems;
> +QString m_group;
> };
Rename to m_groupName ?
> kfileplacesmodel.cpp:474
> +// return a sorted list based on groups
> +qStableSort(items.begin(), items.end(), [](KFilePlacesItem *itemA,
> KFilePlacesItem *itemB) {
> + return (itemA->groupType() < itemB->groupType());
Might be worth breaking the line after "items.end(),"
> kfileplacesview.cpp:62
> public:
> -KFilePlacesViewDelegate(KFilePlacesView *parent);
> +KFilePlacesViewDelegate(int sectionRole, KFilePlacesView *parent);
> virtual ~KFilePlacesViewDelegate();
I don't see the point of that ctor change. KFilePlacesView(Delegate) is already
very much coupled to KFilePlacesModel anyway, so could be hardcoded to
KFilePlacesModel::GroupRole.
> kfileplacesview.cpp:104
> +
> +int m_sectionRole;
> +
Likewise, can go away in my opinion.
> kfileplacesview.cpp:342-356
> +if (index.row() == 0) {
> +return true;
> +}
> +
> +const QAbstractItemModel *model = index.model();
> +QVariant section = index.data(m_sectionRole);
> +QModelIndex prevIndex = model->index(index.row() - 1, index.column(),
> index.parent());
I'd consider refactoring this by splitting it in several helper functions:
- one which returns the groupName from an index (and so an empty string if the
passed index is invalid);
- one which returns the previous visible index starting from an index (pretty
much encapsulating the loop you got);
The whole block would then turn into something like:
const auto groupName = groupNameFromIndex(index);
const auto previousGroupName = groupNameFromIndex(previousVisibleIndex(index));
return groupName != previousGroupName;
More elegant and readable in my opinion, also make sure the whole method is at
the same abstraction level.
> kfileplacesview.cpp:1172
> +
> +for(int i=0; i < q->model()->rowCount(); i++) {
> +if (!q->isRowHidden(i)) {
Spaces before and after =
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure, ervin
Cc: ervin, anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
anthonyfieroni added inline comments. INLINE COMMENTS > kfileplacesview.cpp:104 > + > +int m_sectionRole; > + const > kfileplacesview.cpp:1065-1066 > const int maxWidth = q->viewport()->width() - textWidth - 4 * margin - 1; > -const int maxHeight = ((q->height() - (fm.height() / 2) * rowCount) / > rowCount) - 1; > +int maxHeight = ((q->height() - (fm.height() / 2) * rowCount) / > rowCount) - 1; > +maxHeight = sectionsCount() * fm.height(); > So if second line is correct why not remove first one? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks, dfaure Cc: anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo updated this revision to Diff 20613. renatoo added a comment. Does not use KCategorizedView Instead of use KCategorizedView, draw the section over the first item area REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8243?vs=20587=20613 BRANCH model-hide-row REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, dfaure Cc: anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
anthonyfieroni added a reviewer: dfaure.
anthonyfieroni added inline comments.
INLINE COMMENTS
> kfileplacesview.h:37
> */
> -class KIOFILEWIDGETS_EXPORT KFilePlacesView : public QListView
> +class KIOFILEWIDGETS_EXPORT KFilePlacesView : public KCategorizedView
> {
You cannot do that, you have better chance to win a lottery than to keep ABI
compatibility. Better approach is to make changes in delegate.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8243
To: renatoo, #frameworks, dfaure
Cc: anthonyfieroni, cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
cfeck added a reviewer: Frameworks. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8243 To: renatoo, #frameworks Cc: cfeck, #frameworks
D8243: Implement support for categories on KfilesPlacesView
renatoo created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Make use of KCategorizedView on file dialogs to classify the entries in groups, most similar as possible with dolphin TEST PLAN From an kde app try to open/save a file. It should show a new file dialog with categories on the places planel REPOSITORY R241 KIO BRANCH model-hide-row REVISION DETAIL https://phabricator.kde.org/D8243 AFFECTED FILES src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp src/filewidgets/kfileplacesview.h To: renatoo Cc: #frameworks
