KIO: make file dialog columns resizable again (and movable)
Needs RevisionPublic

Authored by rjvbb on Jan 19 2019, 10:10 AM.

Details

Summary

File dialog detailed view columns were made fixed-size in commit e504bc1fd56412ee7e9748a0dfafa537977ec1b5, leading to bug 401506.

This patch restores the interactive mode users expect in detailed file list views but preserves the "ideal" column sizing.

Qt doesn't allow combining modes (QHeaderView::Stretch|QHeaderView::Interactive) so a bit of a ruse is required to activate interactive mode after the definitive sizes have been set. This patch achieves that as follows:

1 A slot is connected to QHeaderView::sectionResized when a Polish event is received, and state member variables are initialised.
2 The handler (slot) stores new, positive sizes for individual columns (sections) under condition that the QTreeView contains entries. No attempt i made to change the QHeaderView settings directly from here. All this happens before the widget will be drawn (evidently(?)).
3 The state information is checked just before the widget is to be drawn (when the 1st Paint event is received); if sizing information is available the corresponding columns are resized to their respective stored sizes explicitly, and the entire QHeaderView is set to interactive resize mode.

In my testing the first paint event always comes after the final column sizes have been determined, and it is not too late to change resize mode here.

Currently the handler slot from 1) is a lambda insteead of a member function. A member function could be disconnected once the required information is obtained and acted upon but handler overhead is negligible and file dialogs are typically not long-lived.

Possible and welcome future development: save and restore user-defined column sizes. The resize handler would have to remain in place for that (and track new sizes signalled when resizeMode==Interactive).

BUG: 401506

Test Plan

Works as intended on Mac and Linux/X11 with FW. 5.52.0 and Qt 5.9.7 (I see no reason why this would behave differently on different platforms).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7392
Build 7410: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
rjvbb requested review of this revision.Jan 19 2019, 10:10 AM
rjvbb updated this revision to Diff 49864.Jan 19 2019, 10:17 AM

I realised the 1st version introduced a regression. This update restores automatic sizing when the dialog is resized.
The fix currently has the effect of undoing any column sizing the user may have done before resizing the entire dialog, but that doesn't seem entirely unjustified. It won't be hard to preserve the interactive resize mode for sections that have a custom size but AFAIAC we can put off implementing that until there's a clear demand for it.

rjvbb set the repository for this revision to R241 KIO.Jan 19 2019, 10:17 AM
rjvbb updated this revision to Diff 49869.Jan 19 2019, 11:47 AM

Setting explicit sizes is best done in a mode that supports doing so... =/
Also, sections can be movable (again) now (I often like to see dates first, sizes 2nd).

rjvbb retitled this revision from KIO: make file dialog columns resizable again to KIO: make file dialog columns resizable again (and movable).Jan 19 2019, 11:47 AM
rjvbb set the repository for this revision to R241 KIO.

This does indeed make the Name column resizable again (+1) but when the view is very narrow, it still starts out with a poor size by default. Maybe we can incorporate some more intelligence here and set a sensible default width for the Name column when the height of the view is greater than the width.

rjvbb added a comment.Jan 19 2019, 6:41 PM

Maybe we can incorporate some more intelligence here

Same commit? It is a separate issue in a sense, no - you say "it *still* starts" so not a regression I introduced?

and set a sensible default width for the Name column when the height of the view is greater than the width.

Danger! What happens if you maximise the dialog vertically (I have WM shortcuts for that and use them often)?! I think that no one would expect that the name column starts behaving differently all of a sudden when you do that.

The problem here is that as far as I can tell we cannot ask Qt to calculate the column outside of the normal displaying loop, and the dialog doesn't help by not adding all items at once. Maybe a minimum width can be set (during the auto-sizing phase, to be lifted when interactive mode is enabled)?

How "very narrow" are we talking about, and to what extent is this a real-life issue? I myself tend not to be amazed when a widget like this gets garbled when resized too small. Esp. when it's like here where 2 columns remain at the same size and the left most just keeps getting smaller.

Thought: if the width of the name column becomes less than the width of the date column, reduce the size of the date and size columns. Question is, by how much, and will it work to do that in the resize handler/slot. This can also be done just before interactive mode is activated. For instance, reduce the date column by 1/3rd, the size column by 50% and add the recuperated pixels to the name column width. I'll have a look tomorrow how that behaves.

ngraham added a comment.EditedJan 19 2019, 9:03 PM

Maybe we can incorporate some more intelligence here

Same commit? It is a separate issue in a sense, no - you say "it *still* starts" so not a regression I introduced?

Well, I would say this patch doesn't actually resolve the issue yet. Making the headers manually resizable again is good, but providing a good default size so that users don't have to is better. Without that, we'll still get bug reports about the bad default size in vertical sidebars (e.g. Kate's filesystem browser plugin)

Danger! What happens if you maximise the dialog vertically (I have WM shortcuts for that and use them often)?! I think that no one would expect that the name column starts behaving differently all of a sudden when you do that.

If there's still enough room to show everything without a horizontal scrollbar, then we're still good. If not, then it should start showing a horizontal scrollbar and pick a reasonable-ish width for the name column.

The problem here is that as far as I can tell we cannot ask Qt to calculate the column outside of the normal displaying loop, and the dialog doesn't help by not adding all items at once. Maybe a minimum width can be set (during the auto-sizing phase, to be lifted when interactive mode is enabled)?

How "very narrow" are we talking about, and to what extent is this a real-life issue?

It is a very significant real-life issue for vertical sidebars, such as Kate's Filesystem Browser plugin when showing Detailed View or Detailed Tree View. Play around with that and see if you can come up with something that makes those views look good out of the box with no adjustment needed.

Basically we want for this widget to display a sane view out-of-the-box with the file dialog and vertical sidebars, fall back to a reasonable behavior if there still isn't enough horizontal space, and finally allow the user to resize the headers manually as a last resort.

rjvbb updated this revision to Diff 49908.Jan 19 2019, 10:08 PM

tentative fix for "very narrow views".

This imposes a simple minimum width on the name column. The value is taken from (33% wider than) the date column which has a useful width (which is already based on the current font). An additional compensation is applied to the size column which is made 20% narrower. This may not be entirely necessary; as an alternative we could suppress it completely for this kind of view?
(It would also be useful if the tooltips were column specific!)

rjvbb set the repository for this revision to R241 KIO.Jan 19 2019, 10:11 PM
If there's still enough room to show everything without a horizontal scrollbar, then we're still good.

No, resizing a view so that it becomes taller than wide shouldn't make it behave as if it's become very narrow all of a sudden. Similarly, a very narrow view isn't always taller than it's high (you could have a very small file side-bar in a dolphin window, for instance).

I've uploaded a new version which has what I think is probably the best and easiest option.

It is a very significant real-life issue for vertical sidebars

Yes, of course, I didn't realise those use the same widget.

This version works pretty well!

I'm not sure I like the bit where you make the Size column 20% smaller under certain circumstances though. Since the Size column was previously sized perfectly, making it 20% smaller makes all of its items get elided and become useless. Removing that bit makes the behavior feel better to me since the view just gets a scrollbar a bit sooner, which I think it a nicer behavior than making the Size column useless.

With that change, I'd be pretty close to a "ship it!"

rjvbb added a comment.Jan 20 2019, 9:56 AM

I agree that reducing the size column width isn't as easy as I thought. I find it takes up more space than necessary when horizontal space is at a premium and I was hoping to get a more compact read-out that can still be interpreted ... and that would show itself in full in a tooltip.

Maybe something can be done at a higher level, just like some systems will switch from full to compact date format when available space requires it (Mac OS will even switch to a narrow font). I'm not very motivated though to spend much time in figuring out how and where to do such a thing so if it's not straightforward I'll be leaving that approach to someone else.

One thing we might be able to do is use QFont::setStretch() to make the text a bit more compact. I'll follow that thought for a bit.

rjvbb updated this revision to Diff 49947.Jan 20 2019, 4:34 PM

Final attempt at being a bit clever :)

Entering "narrow mode" now also sets the font stretch (exitting the mode will reset the stretch), the same as the Mac Finder will do. I use factor 83, which corresponds roughly to making a 12pt font as wide as a 10pt font.
Setting stretch does not seem to have any effect when done in the handler slot, so it has to be done together with the width adaptation. That means the size and date columns need to be stretched too otherwise they will be too wide; I find that condensing them by half as much as the font gives a satisfactory result. The gained pixels are added to the name column.

Tested with a range of fonts and font sizes I find this to work pretty well. NB: the default UI font doesn't seem to support stretch; the code detect this by comparing QFontMetrics::avCharWidth() before and after setting the stretch.

The look *is* slightly different with the condensed font, but theoretically it's only a quantitative difference ;)

rjvbb added a comment.EditedJan 20 2019, 4:40 PM

My QtCurve-based theme, "narrow mode" just activated:


Idem, "narrow mode" just deactivated

Default theme, narrow mode just activated


Idem, narrow mode just deactivated

In-situ, in Kate's filesystem browser:
with my own style+fonts where stretching has an effect:

and with the default/stock theme+fonts (Noto Sans) where stretching does not have effect:

I don't see the squeezed text when using Breeze and Noto Sans. Am I missing something?

I kind of hope so, because I predict that if we ship with text that gets squeezed at various sizes, we'll totally get bug reports about it. :-) No need to overcomplicate things IMHO. The prior state of the patch seemed just about perfect to me with the removal of the Size column shrinking.

I don't see the squeezed text when using Breeze and Noto Sans. Am I missing something?

No, I don't think so. This must correspond to the 2nd pair of images I attached.

I kind of hope so, because I predict that if we ship with text that gets squeezed at various sizes, we'll totally get bug reports about it. :-)

No, this is mostly a font-specific thing; Noto Sans apparently doesn't stretch (nor squeeze) at typical UI sizes.
And do you know of examples where this widget does NOT use the generic text font for the directory list? IOW, what are the chances that a single user can get instances of this widget where the font doesn't stretch and other instances where the font does stretch (because of being at another size or family)? Would they even notice?

I kind of agree with the principle of not complicating things overly, but I also kind of like the effect I created here. Who could we add to this review to get some more opinions?

I have a little font tinker app at github:RJVB/fontweightissue-qt5 (and a plasma-integration patch that adds support for alternative "kdeglobals" configurations at https://github.com/RJVB/macstrop/blob/master/Linux/kf5/kf5-plasma-integration/files/patch-support-alt-config.diff ;) )

FWIW, there's also a possibility to reduce letter spacing which can help here, but there the margin for error is a lot smaller.

Oh sh@@t, I understand what's happening here.

The default look-and-feel still appends the Regular stylename to the font, and since font stretch is a programmatic way to set a font style ("condensed") this too is overridden when a stylename is set.

That does NOT apply to letterspacing.

Noto Sans, left 83% stretch, right 88% letterspacing (same width reduction):

Seeing this now it does look more like what I see in the Mac GUI, and probably looks less different than a condensed font. I'll check this in the code later today.

If this code depends on https://bugs.kde.org/show_bug.cgi?id=378523 being fixed, then we need to fix that first. The discussion in that bug kind of petered out, unfortunately. I think you're the only one who understands what's going on there, so would you care to have another go at fixing it?

Alternatively, we could ship the part of this patch that doesn't depend on font squeezing, and then ship that part later once 378523 is fixed.

rjvbb added a comment.Jan 21 2019, 8:46 PM

The discussion in that bug kind of petered out, unfortunately.

It did, and indeed. I thought a fix had been implemented for KFontRequester, but either I'm mistaken, or I have a fix locally.

I think you're the only one who understands what's going on there, so would you care to have another go at fixing it?

I seem to recall that some people (the Plasma guys?) thought that the stylename should be specified. At least my fix to plasma-integration (D9070) didn't remove the stylenames from the DefaultFontData table. That's how I realised what was happening here: I forced a default font config by removing all the font configs from my kdeglobals, and thus got the fonts from that table - styleNames included.
I

Alternatively, we could ship the part of this patch that doesn't depend on font squeezing, and then ship that part later once 378523 is fixed.

I'm close (hopefully) to having a working alternative based on letterspacing. That property is not affected by stylenames and shouldn't be affected by FontConfig settings either (space between letters is more a text rendering than a font property).

rjvbb updated this revision to Diff 50032.Jan 21 2019, 10:55 PM

This version uses letterspacing rather than font stretch, so works regardless of whether a stylename has been set on the font.

Letterspacing reduction has a stronger impact on readability than stretch (if you cannot know the average default letter spacing) so there cannot be as much gain from it.

rjvbb set the repository for this revision to R241 KIO.Jan 21 2019, 10:55 PM
ngraham requested changes to this revision.Jan 21 2019, 11:08 PM

Sorry, I really don't think this text squeezing feature is actually desirable. Here's how it works in Kate:

The transitions are jarring and seem to happen at arbitrary points. The result is really unattractive. And the space gained by all of this is practically nothing. Let's forget about squeezing the text and ship without it. No need to overcomplicate things when we already have a good solution! :)

This revision now requires changes to proceed.Jan 21 2019, 11:08 PM
rjvbb added a comment.Jan 22 2019, 9:22 AM

Yikes, and I can reproduce that. Did you notice this with previous versions that used font stretch? (Probably not if stretch had no effect for the font(s) you tried it with...)

I'm still going to try to fix this; even if in the end it doesn't go in there's be a properly working version on record here.

rjvbb added a comment.Jan 22 2019, 1:34 PM

I'm still going to try to fix this

Good thing I did (am doing), because what you call the jarring exists also without font stretching. It's something in Qt that somehow doesn't occur in file dialogs but only with applications of the widget like in Kate. It is caused by the name column being set to a sort of minimum size, for now beyond my control.

I now have it to the point where the state only occurs transiently when you resize the Kate sidebar. That's still annoying so I'm looking into what can be done through a custom subclass of QHeaderView. There's only 1 virtual method I can override and have called by QTreeView but that may be exactly the one we need.

rjvbb updated this revision to Diff 50083.EditedJan 22 2019, 10:37 PM

Well, that was "interesting".

It turns out that Qt has what looks like another path through which section sizes are calculated and through which they're shown, which apparently isn't used in file dialogs but which sometimes gets used when the detail view widget is used in a (Kate) side-bar. That path could give (much) smaller name column sizes than the designated minimum but also somewhat larger sizes. All this led to the "jarring effect" seen in Nate's video, which did not have anything to do with the font being squeezed.

I've been able to catch the smaller sizes by subclassing QHeaderView and overriding sectionSizeFromContents() and by calling QTreeView::resizeColumnToContents(0) when resizing; when a larger name column width is determine while in "narrow mode" that value now becomes the designated minimum so that the column doesn't "hesitate" between the 2 values (more than once).
Calling QTreeView::resizeColumnToContents can reduce resizing reactivity noticeably so the call is omitted when the widget is used in a file dialog

The font squeezing feature has been dropped, and the code cleaned up. Notably, most of the new variables have been moved to the new KDirHeaderView class, and the section resize handler slot has been promoted from a lambda to a method of that class.

rjvbb set the repository for this revision to R241 KIO.Jan 22 2019, 10:38 PM
rjvbb updated this revision to Diff 50084.Jan 22 2019, 10:43 PM

the change to the headerfile was now redundant.

rjvbb set the repository for this revision to R241 KIO.Jan 22 2019, 10:43 PM

The behavior is better now, thanks. I think it will be sufficient to fix the bug and not generate user complaints about anything!

I'll let someone else do the code review. Maybe someone from Frameworks or Dolphin? That said, one thing sticks out at me:

src/filewidgets/kdiroperatordetailview.cpp
219

This is almost always wrong. Can you explain why you think it's required here?

rjvbb added a comment.Jan 23 2019, 9:10 AM

The behavior is better now, thanks.

It's back to what you liked before I started tinkering with font squeezing (plus a few fixes to the behaviour in side-bars).

Do you know of other applications that use this widget/mode for/in a filebrowser side-bar thingy or otherwise in situations where it might have unexpected behaviour? Neither Dolphin nor KDevelop seem to use.

src/filewidgets/kdiroperatordetailview.cpp
219

I didn't think anything, I did what an error message told me.

When I remove the include it comes back:

[ 78%] Automatic MOC for target KF5KIOFileWidgets
cd /path/to/build/src/filewidgets && /opt/local/bin/cmake -E cmake_autogen /path/to/build/src/filewidgets/CMakeFiles/KF5KIOFileWidgets_autogen.dir/AutogenInfo.cmake MacPorts
AutoMoc error
-------------
  "/path/to/kio-git/src/filewidgets/kdiroperatordetailview.cpp"
The file contains a Q_OBJECT macro, but does not include "kdiroperatordetailview.moc"!
Consider to
 - add #include "kdiroperatordetailview.moc"
 - enable SKIP_AUTOMOC for this file

make[2]: *** [src/filewidgets/CMakeFiles/KF5KIOFileWidgets_autogen] Error 1

It must be due to the fact that I put the entire KDirHeaderView class definition into the implementation file, which I did because it's really private and changes to it shouldn't trigger a rebuild of a large number of files that are not concerned by those changes at all (kdiroperatordetailview_p.h is included by quite a few files).

rjvbb added a subscriber: kwrite-devel.
ngraham accepted this revision.Jan 23 2019, 11:00 PM
ngraham added reviewers: Dolphin, apol.

The behavior is better now, thanks.

It's back to what you liked before I started tinkering with font squeezing (plus a few fixes to the behaviour in side-bars).

Much nicer. I like it! Thanks for the explanation regarding the moc change. Seems sane. Code overall seems sane, but please wait for a more in-depth review from someone else before committing.

Do you know of other applications that use this widget/mode for/in a filebrowser side-bar thingy or otherwise in situations where it might have unexpected behaviour? Neither Dolphin nor KDevelop seem to use.

Check this out: https://lxr.kde.org/search?_filestring=&_string=KDirOperator&_casesensitive=1

A few others I know off the top of my head are Okteta and K3B.

This revision is now accepted and ready to land.Jan 23 2019, 11:00 PM
rjvbb added a comment.Jan 24 2019, 8:22 AM

David, Andreas, any idea why the name column all of a sudden jumps to a larger width when the widget is used in a side-bar and you're making the view narrower and approach the minimum width? It works in our favour here because the end result is that the name column becomes about as wide as the view itself (and I ensure it won't change size again).
It just nags me a bit that I haven't been able to figure out why it happens...

I'm not against this change, but have the feeling another +2 from someone who know this stuff is a good idea.

cfeck added a subscriber: cfeck.Jan 24 2019, 10:35 AM
cfeck added inline comments.
src/filewidgets/kdiroperatordetailview.cpp
53

Can we use qobject_cast here?

rjvbb marked an inline comment as done.Jan 24 2019, 1:57 PM
rjvbb added inline comments.
src/filewidgets/kdiroperatordetailview.cpp
53

Doh, of course.

It's probably more expensive (which shouldn't matter here) but also means we have to include kfilewidget.h. Is that OK?

cfeck added a comment.Jan 24 2019, 2:32 PM

As far as I know, using qobject_cast is faster than comparing class names, because it only compares metaclass pointers. Additionally, it allows subclasses.

rjvbb added a comment.Jan 24 2019, 3:14 PM
As far as I know, using qobject_cast is faster than comparing class names, because it only compares metaclass pointers. Additionally, it allows subclasses.

Purely academic: that would be true for an ObjC-like construct like isKindOfClass but doesn't it qobject_cast also do additional work as part of the casting?
Allowing subclasses could be good here, but it could also defeat the purpose, that'd depend on what the subclass changes w.r.t. KFileWidget.

rjvbb updated this revision to Diff 50205.Jan 24 2019, 6:49 PM

Use qobject_cast.

rjvbb set the repository for this revision to R241 KIO.Jan 24 2019, 6:49 PM
markg requested changes to this revision.Jan 27 2019, 4:15 PM
markg added a subscriber: markg.

I too would quite like the to have resizeable columns back.
But the approach you've chosen looks a little too complicated. Also, the fact that KDirOperatorDetailView::event needs to know about your custom QHeaderView is a code smell. Sometimes you do indeed need logic like that to make things work. But still, isn't there another way? Now the header and view are locked together. One doesn't work without the other.

The thing that triggered me to comment wasn't any of that though. It's the "narrow mode". I cannot see how that wouldn't be seen as a bug by a user. With that little trick you're also overruling any font spacing settings the user might have had (in fontconfig) which is quite likely going to cause unexpected behavior and therefore bug reports.

Now let's take a step back. What do we really want? We want:

  • Have the first column resizable and calculate the optimal size when the user had not resized the column yet.
  • Restore that setting if the user had manually resized it.

In QHeaderView code, what we want is "QHeaderView::Interactive" [1] followed by QHeaderView::resizeSection [2]. Exactly as [1] described! QHeaderView merely lacks a convenience function for this, that is what we have to implement!
I quickly threw this in a test project (minus the state saving) and i ended up with this fairly minimal QHeaderView subclass [3: header], [4: source] and this sample to see how you use it [5]. Note that there is one quirk in there in the source. The size calculation is not perfect and will fail on longer strings! That likely needs to be extended to take the style and margins into account. Anyhow, the goal of this example is to show a different approach with a far smaller code footprint and be far easier to debug. Use it as you please :)

[1] http://doc.qt.io/qt-5/qheaderview.html#ResizeMode-enum
[2] http://doc.qt.io/qt-5/qheaderview.html#resizeSection
[3] https://p.sc2.nl/r1RJBLsQ4
[4] https://p.sc2.nl/BJlMSLimN
[5] https://p.sc2.nl/SJIYI8jQV

This revision now requires changes to proceed.Jan 27 2019, 4:15 PM

In QHeaderView code, what we want is "QHeaderView::Interactive" [1] followed by QHeaderView::resizeSection [2]. Exactly as [1] described! QHeaderView merely lacks a convenience function for this, that is what we have to implement!

Using that alone results in a visual and functional regression of the original problem we were trying to fix (that file dialogs don't have a sensible default view):

If we go with this route, I would want to make sure we preserve the current behavior of other columns being right-aligned, and also make sure that the Name column has a good default width. If we can ensure that, I'm all for it!

rjvbb added a comment.Jan 27 2019, 5:22 PM

But still, isn't there another way? Now the header and view are locked together. One doesn't work without the other.

What's the problem with that? The custom header class isn't public. I did indeed use it for stuff that were not part of a class, or of the KDirOperatorDetailView class in earlier iterations. There's no hard reason for that, I just find it tidier (and it saves me from having to compile all other modules that import the kdiroperatordetail view headerfile when I change a detail that doesn't concern them).

The thing that triggered me to comment wasn't any of that though. It's the "narrow mode". I cannot see how that wouldn't be seen as a bug by a user. With that little trick you're also overruling any font spacing settings the user might have had (in fontconfig) which is quite likely going to cause unexpected behavior and therefore bug reports.

I'd say make a test case and convince us. You may have missed the fact that I'm no longer doing anything to the used font (the same in all columns). I'm just using the event flow to let Qt determine column widths using whatever information it wants, and then I "fixate" those widths in order to restore interactive mode. This is an admittedly complex way to do something that Qt doesn't allow us to do: 1) set interactive mode, 2) ask QHeaderView for the width that would be used in one of the automatic modes, 3) set those modes (with a minimum imposed on the name column).

I can imagine that someone might file a bug about the name column getting a bit larger when resizing. In practice I'm not convinced many users will even notice this because it will happen only once, making the column more readable (and how much time does one spend resizing side-bars anyway). I think this is a bridge to take if and when we get there. A possible workaround would be to calculate our own minimum width in such a way that we arrive at the width Qt later decides to pick for some reason (for the default font at least).

Something we (= a number of us) need to look into is what happens when you resize a view *after* a manual column adjustment, and what should happen in that case. Knowing that anything to make the manual adjustment permanent will only increase the code complexity.

The size calculation is not perfect

My initial implementation wasn't perfect either (Nate complained how it worked in narrow mode) ... and addressing that was what introduced most of the complexity.

At this point I have invested enough time and energy in what is essentially a behavioural detail. I like doing that kind of thing but I'm not motivated enough to start from scratch (I'll consider making little changes but not much more).

markg added a comment.Jan 27 2019, 5:40 PM

In QHeaderView code, what we want is "QHeaderView::Interactive" [1] followed by QHeaderView::resizeSection [2]. Exactly as [1] described! QHeaderView merely lacks a convenience function for this, that is what we have to implement!

Using that alone results in a visual and functional regression of the original problem we were trying to fix (that file dialogs don't have a sensible default view):

If we go with this route, I would want to make sure we preserve the current behavior of other columns being right-aligned, and also make sure that the Name column has a good default width. If we can ensure that, I'm all for it!

Lol :)
It wasn't meant as a direct drop-in replacement for Rene's patch. KIO probably sets more on the headerview that isn't applied now (i assume). And if it doesn't then some things do need to be set to make it look nice. It's merely meant to demonstrate that you can change the width of a column to whatever you desire while keeping the resizable feature and without the need to catch events and do magic there.

Anyhow, you basically want a "stretchFirstColumn" while keeping. I named it "stretchColumn" but as it is now it only works if one column uses it. You need more complex layouting to get that to work if you want to stretch over multiple columns. But that's not the case here.
Here it is. [1: header], [2: source], and a example on how to use it [3]. This obviously is far from done, but again only to demonstrate. If you like this approach i can put some time in it to make it less sensitive to errors.
be aware to call "stretchColumn" after you show the dialog! Otherwise the sizes haven't been calculated yet. If you do exec, you can get around it by doing a QTimer::singleShot.

[1] https://p.sc2.nl/H1S-2DomE
[2] https://p.sc2.nl/ryQ2hwoX4
[3] https://p.sc2.nl/Bybanvj7V

markg added a comment.Jan 27 2019, 6:06 PM

But still, isn't there another way? Now the header and view are locked together. One doesn't work without the other.

What's the problem with that? The custom header class isn't public. I did indeed use it for stuff that were not part of a class, or of the KDirOperatorDetailView class in earlier iterations. There's no hard reason for that, I just find it tidier (and it saves me from having to compile all other modules that import the kdiroperatordetail view headerfile when I change a detail that doesn't concern them).

The thing that triggered me to comment wasn't any of that though. It's the "narrow mode". I cannot see how that wouldn't be seen as a bug by a user. With that little trick you're also overruling any font spacing settings the user might have had (in fontconfig) which is quite likely going to cause unexpected behavior and therefore bug reports.

I'd say make a test case and convince us. You may have missed the fact that I'm no longer doing anything to the used font (the same in all columns). I'm just using the event flow to let Qt determine column widths using whatever information it wants, and then I "fixate" those widths in order to restore interactive mode. This is an admittedly complex way to do something that Qt doesn't allow us to do: 1) set interactive mode, 2) ask QHeaderView for the width that would be used in one of the automatic modes, 3) set those modes (with a minimum imposed on the name column).

I know. I'm not attacking you here :) I've been in the same annoying situation countless of times when wanting to have this very feature. It's a pain Qt doesn't give it. But there are ways to solve it and yours is on the complicated side.

I can imagine that someone might file a bug about the name column getting a bit larger when resizing. In practice I'm not convinced many users will even notice this because it will happen only once, making the column more readable (and how much time does one spend resizing side-bars anyway). I think this is a bridge to take if and when we get there. A possible workaround would be to calculate our own minimum width in such a way that we arrive at the width Qt later decides to pick for some reason (for the default font at least).

This is a typical "last mile" thingy. Yes, it works. But not as perfect as it could be which eventually is going to add up in hundreds of other "last mile" things. Each on their own not worth the time to fix. All together giving the user a "yep, they still need to work uit some bugs" feeling.

Something we (= a number of us) need to look into is what happens when you resize a view *after* a manual column adjustment, and what should happen in that case. Knowing that anything to make the manual adjustment permanent will only increase the code complexity.

I'd say the last column will be the victim there. Just enable setStretchLastSection(true) (works in my example too) and let a resize be handled that way. This results in the view leaving all columns exactly as is when resizing except for the last column. The user still has to resize the name column to make it bigger/smaller and that very action should be stored and restored next time. It saves the hassle of trying to figure out what the user intended.

The size calculation is not perfect

My initial implementation wasn't perfect either (Nate complained how it worked in narrow mode) ... and addressing that was what introduced most of the complexity.

At this point I have invested enough time and energy in what is essentially a behavioural detail. I like doing that kind of thing but I'm not motivated enough to start from scratch (I'll consider making little changes but not much more).

Your work is well appreciated! It triggered me to look into it as well. Together we can probably figure out a perfect solution that does't leave any hiccups that the user will consider a bug, however short they might be. And as an added bonus might be used as a generic better QHeaderView version.

p.s. This custom header view thing should definitely be a candidate for probably KItemViews. Not my version as-is, but the concept of having more flexibility in the header view.

rjvbb added a comment.Feb 11 2019, 9:24 AM

So this is just going to become a victim of the better-is-the-enemy-of-good principle?

I'm not saying there can be better ways to do this but AFAIC this is already pretty good, certainly good enough and even too good in practice to keep me from more interesting things to come back and look at other implementations.

So I'm really inclined either to push this in a couple of days in accordance with Nate's green light, or else abandon things and keep using my own solution until someone else pushes a fix (that suits me just as well).

I hope not, but that's the problem with the wall-of-text communication style. Eventually people get exhausted and the discussion peters out with nothing getting done.

What's the minimum viable change here?

rjvbb added a comment.Feb 12 2019, 9:55 AM
What's the minimum viable change here?

I think my the implementation of my solution is pretty minimal by now, no? :)

@David Faure: any ideas/suggestions, seems right up your alley?

I hope not, but that's the problem with the wall-of-text communication style. Eventually people get exhausted and the discussion peters out with nothing getting done.

...aaaaaand that's exactly what happened. :(

What needs to get done here to land this?

rjvbb added a comment.Mar 14 2019, 5:43 PM
...aaaaaand that's exactly what happened. :(

I'm not exhausted, my patch just works good enough (thanks also to feedback on here) to make me more interested in living the rest of my life rather than continue to explore alternatives. Maybe if an alternative patch review were posted (and mentioned) here I'd get myself to take a look, but maybe we'll only get one after this proposition has been committed and tested in the wild.

Give it a couple of days. Then, if no one else reacts, decide what you prefer: the current fixed-width columns or my possibly suboptimal implementation that makes them resizable again.

dfaure requested changes to this revision.May 9 2019, 8:59 AM

I didn't read the full encyclopedia of discussions here, I only looked at the patch.

But really, this is far too hacky to my taste. If QHeaderview is indeed missing a feature, it might be best to implement it there, it will be much cleaner.

src/filewidgets/kdiroperatordetailview.cpp
50

This is a horrible hack, relying on the exact number of parents before getting to KFileWidget.
Either cast inside a while loop (i.e. "go up until a KFileWidget"), or find a way to pass the proper pointer to this class.

77

use override, not virtual

201

Why is this (and the following 12 lines or so) done on every resize? Surely that's not needed?

Split this into case Resize and case Polish, I don't see even one line of code that needs to be in both.

214

URGGH. This smells like a horrible hack. Widgets do layouting, and then painting. Modifying layouting parameters (hiding columns, resizing columns...) at painting time is very wrong and a recipe for infinite loops (paint -> layout -> paint -> layout...).

The comment talks about a "first paint event" - if that's all you need, then at least add a static bool so that this code is run only once. But I still won't like it very much. Isn't that what Polish is for?

219

Yeah, this is correct when defining a class with Q_OBJECT in the cpp file. The moc code needs to see the class definition, and the only way to do that is to include the moc.

260

If you mean Q_ASSERT, use Q_ASSERT. If on the other hand, it can happen for good reasons, remove the qWarning.

rjvbb added a comment.May 9 2019, 10:11 AM
I didn't read the full encyclopedia of discussions here, I only looked at the patch.

It's really a pity that you only did that now, because by now I'll have to reverse-engineer my patch (and read the encyclopedia) to remember how I arrived at the patch.

But really, this is far too hacky to my taste. If QHeaderview is indeed missing a feature, it might be best to implement it there, it will be much cleaner.

Yes, it has a good perfume of a proper hack, I won't disagree with that. But sometimes that's unavoidable if the cleaner solution is by definition a long-term one (that's unlikely to appear in the current Qt LTS version, for instance).

This is a horrible hack, relying on the exact number of parents before getting to KFileWidget.

I'm not sure I agree. IIRC it's based on knowledge of how the standard KIO filedialog is constructed, which seems OK in another part of KIO code.

Either cast inside a while loop (i.e. "go up until a KFileWidget"), or find a way to pass the proper pointer to this class.

You'd probably want to put an additional end condition to that loop, no?

Why is this (and the following 12 lines or so) done on every resize? Surely that's not needed?

If memory serves me well it may not be needed on every loop indeed, but it is also not UNneeded on any loop (i.e. sometimes it is).

URGGH. This smells like a horrible hack. Widgets do layouting, and then painting. Modifying layouting parameters (hiding columns, resizing columns...) at painting time is very wrong and a recipe for infinite loops (paint -> layout -> paint -> layout...).

Maybe, but apparently not for the last months in my own test kitchen ...

Isn't that what Polish is for?

I am tempted to say that I should have seen polish events arrive at an appropriate time during the mentioned event analysis. I'm not unfamiliar with those thanks to my work on QtCurve, so I'd hope that it would have occurred to me to exploit them.

If you mean Q_ASSERT, use Q_ASSERT. If on the other hand, it can happen for good reasons, remove the qWarning.

No, I most definitely didn't mean Q_ASSERT. I won't ever knowingly use anything that leads to a crash/abort if there's even a chance that the situation can be handled more gracefully.

All in all I notice I have lost much if not all of my appetite to do much more on this beyond the couple of simpler suggestions made here... I haven't even found the time to update my installs from 5.52.0 (and am still on Qt 5.9 too, at that (though with some stuff backported from 5.10).

Anyone wanting to take over ("commandeer") this patch, please feel free!!

meven edited the summary of this revision. (Show Details)May 10 2020, 7:56 AM