Organize PlacesViews in groups and show section headers similar as
dolphin.
Details
Details
- Reviewers
dfaure ervin ngraham - Group Reviewers
Frameworks VDG - Commits
- R241:7a6eff3fce41: Implement support for categories on KfilesPlacesView
From an kde app try to open/save a file.
It should show a new file dialog with categories on the places planel
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 421 | So you mean sizeHint have a right height ? This height store looks like a workaround. | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 421 | 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. | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 421 | 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. | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 1149–1152 | Now i see i'm in wrong :) | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 168 | 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. | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 168 | Ok i understaind the workaround, but did you know what is the value of opt.index.row() ? | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 168 | 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) | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 168 | 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. | |
Comment Actions
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.
Comment Actions
A couple more changes needed.
| src/filewidgets/kfileplacesitem.cpp | ||
|---|---|---|
| 152–153 | 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. | |
| src/filewidgets/kfileplacesview.cpp | ||
| 122 | 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. | |
| 168 | @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. | |
Comment Actions
updated comit summary
renamed m_dragModeCount to m_dragStart
refactory 'KFilePlacesItem::data' function
Comment Actions
Screenshots, my man! Screenshots! Add them to the Summary section so people can see what they're getting if this gets merged. :)
Comment Actions
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
| autotests/kfileplacesmodeltest.cpp | ||
|---|---|---|
| 323 | too -> to | |
| src/filewidgets/kfileplacesitem.cpp | ||
| 127 | Q_UNREACHABLE() exists for this purpose | |
| src/filewidgets/kfileplacesview.cpp | ||
| 122 | move this bool next to the other one (m_showHoverIndication), to save on padding. | |
| 124 | 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. | |
| 167 | The grammar is a bit strange. "When drawing" or "If we are drawing" and later on "do not draw". | |
| 177 | this if() doesn't seem very useful to me. | |
| 353 | boddy -> body | |
| 460 | Use QApplication::fontMetrics() instead, it'll be much faster. | |
| 737–738 | 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 && ....) | |
| 1259 | Move rowCount to local variable. This is a non-inline virtual method, no way the compiler can know that it's constant. | |
| 1261 | const | |
Comment Actions
+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 ;-)
Comment Actions
@renatoo, can you update any remaining Not Done inline comments that are done now (including the discussion ones)?
Comment Actions
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...
Comment Actions
@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.
Comment Actions
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.
Comment Actions
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. :)
Comment Actions
@renatoo, there are still a few Not Done comments. Any chance you can resolve them so we can push this forward?
Comment Actions
Looks fine code wise now, just a couple more tweaks to make those sections look closer to what Dolphin got.
| src/filewidgets/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. | |
| src/filewidgets/kfileplacesview.cpp | ||
| 460 | 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. | |
| src/filewidgets/kfileplacesmodel.cpp | ||
|---|---|---|
| 476 | 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
}; | |
| src/filewidgets/kfileplacesmodel.cpp | ||
|---|---|---|
| 476 | Fixed enum order and set values for it. | |
Comment Actions
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?
Comment Actions
This will be exposed by this mr: https://phabricator.kde.org/D8367#change-GqgZlwb2MzB8