[PATCH] Tiny patch for spring-loaded folders in Dolphin

[Simon St James]

Hi Peter,

On Sunday 10 August 2008 22:22:35 you wrote:
> Hi Simon,
>
> Am Saturday, 9. August 2008 22:05:38 schrieb Simon St James:
> > Hi all,
> >
> > On Friday 08 August 2008 19:38:00 Peter Penz wrote:
>
> [...]
>
> > Phew - I think I may be out of my depth, here - this is considerably
> > harder than I expected!
>
> Thanks for your efforts :-) Yes, those requests often really turn into a
> lot of work...

It all seemed so simple when it was just a 6-line patch ;)

> > Ok, please find attached the current, much more complex patch.  It mostly
> > works, with one exception - if you drag an item onto a folder that would
> > cause the view showing the dragged item to disappear - for example,
> > dragging from a DolphinIconView into a folder which is set to use
> > DolphinDetailsView, or dragging from a directory X to a sibling Y via X's
> > parent in columns view, then the folder is entered correctly, but the
> > dragging is cancelled, so you can't keep "drilling down", which is
> > disconcerting.  I have no idea where the drag is cancelled, or how and
> > whether this can be fixed.
>
> I don't have an answer for this yet and also must have a look first...

I have some plans, but they are rather hackish.  The basic idea is to delegate 
the final call to <view>->deleteLater() in DolphinColumnView::deleteColumn() 
and DolphinView::deleteView() to a static method 
FolderExpander::deleteWhenNotDragSource (or a better name ;)) which hide()s 
and disconnect()s <view> (which seems not to cancel the drag) but does not 
delete it until it thinks <view> is not a drag source.  It will err on the 
side of caution by deleteLater()-ing <view> if it can't rule it out for sure.  
I'll provide an actual patch for review when I get a chance, but will quite 
understand it if it sounds too hairy to commit ;)

> > The basic idea is to create a class called AutoExpander (incidentally,
> > naming is one of my weak points - please feel free to suggest alternative
> > names!) that can add spring-loaded functionality to most
> > QAbstractItemViews - I preferred this approach to duplicating code in the
> > various class types. The original patch leveraged QTreeView's own
> > auto-expand functionality, but I've now abandoned that as a) auto-expand
> > would have to be written for the other view types anyway and b)
> > QTreeView's auto-expand doesn't play well with the auto-scrolling in the
> > sidebar tree.  The class had to have the
> > LIBDOLPHINPRIVATE_EXPORT directive as treesidebarpage uses it - I'm not
> > sure if this is a problem or not.
>
> I like your approach of having one common class and also think the general
> direction is full valid.
>
> I'm not sure about the name, but let's see... What about just naming it
> FolderExpander? That the expanding is done automatically is not so
> important from my point of view as saying _what_ it expands.
>
> [...]
>
> > There are a few TODOs in there which represent things that I'd appreciate
> > advice on.
>
> Great :-)
>
> > This patch obsoletes the original patch.
>
> Please let me give some short notes to the patch - there's a lot of minor
> nitpicking here, please don't feel offended. As said before: I like the
> overall approach and am glad for your support. I'm also aware that the code
> is in draft and maybe you are already aware about most nitpickers here:

> <snip>

Not offended at all - this is just the kind of very thorough critique and 
improvements I was hoping for! I agree with all but a few (all related to the 
same thing) and will prepare an updated patch within the next 24 hours.

The areas I'm not sure on all pertain to these:


> +        <entry name="AutoExpandAnyView" type="Bool">
> +            <label context="@label">Use auto-expanding folders for all
> view types</label>
> +            <default>false</default>
> +        </entry>
>
> What do you think about: AutoExpandFolders?
...
>
> However I don't like the naming of 'alwaysExpandThisView'. What about
> having just methods like:
>
> void AutoExpander::setEnabled(bool enable);
> bool AutoExpander::isEnabled() const;
>
> instead? The 'alwaysExpandThisView' is unclear from my point of view
> ('always' vs. 'sometimes', 'this view' vs. 'other view'...).

The concept that the "alwaysExpandThisView" and "AutoExpandAnyView" are trying 
to capture is that if this is true, then whether or not a folder in this view 
is expanded does not depend on the current setting of 
DolphinSettings::instance().generalSettings()->autoExpandAnyView() [or the 
renamed version of this ;)].  setEnabled(...) sounds like it will always 
expand the view or always not expand the view, when it actually depends on a 
setting that can change after the FolderExpander is created.  

It's a rather tricky concept to explain in a few characters :/ I'm happy to 
leave the final call up to you, though.

And while I've got your attention ;) - this bug:

https://bugs.kde.org/show_bug.cgi?id=168254

will need some additional things exposed in the DolphinPart, I think: the 
patch (with "HACK HACK HACK" where I'm doing some rather dubious 
workarounds!) is here:

http://lists.kde.org/?l=kfm-devel&m=121821503218290&w=2

and advice would be appreciated :)

Also, I'm wondering what the plans are re: 
https://bugs.kde.org/show_bug.cgi?id=161848 ? I had a quick look today, and 
couldn't really think of a way of doing it without making KToolTipItem a 
QObject and adding a couple of signals and slots to it, which might not be 
the desired course.  No rush on this, though - I have a whole bunch of other 
stuff to be getting on with in the meantime!

Cheers,S
Si.