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

[kdedevel at etotheipiplusone.com]

Hi Peter,

Quoting Peter Penz <peter.penz at gmx.at>:

> [...]
>> > 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 ;)
>
> Postponing the deletion of the view until the dropping has been done  
>   sounds OK
> to me. But using a static method in FolderExpander sounds dangerous: Assuming
> we have the following directory structure:
> A
>   B
>     C
>
> A uses the icons view, B the details view and C the column view. Now the user
> drags an item into A, drags it into B and drags it into C. When is the view A
> deleted internally? Do we have a queue?

Good point.  I think the course of action I'm planning (but haven't
described yet) would have the following property:

- By the end of app termination, at most one view would not have been
deleted, and we would hold a pointer to this view.

but the fact that there is any doubt in my mind about this and that
the reasoning for it is so subtle is a good argument against its
inclusion :/

Incidentally, the reason I'm considering this approach at all ties
into your next comment:

> Just a quick thought: The DolphinView is responsible for creating the views,
> so it also should take care deleting them and we should not forward this to
> the FolderExpander. I think the folder expander should emit a signal when the
> dragging has been finished. By this e. g. the DolphinView can queue the
> pending views that must be deleted and deletes them as soon as it gets the
> signal from the Expander. What do you think?

This was my first thought and would be absolutely ideal, but
unfortunately it is thwarted by the fact that, while a startDrag event
can be reliably detected by FolderExpander, cancellation of the drag
cannot (a successful QDragDrop can be, though, IIRC), as far as I can
tell, which is a major pain the a** ;)

A view can itself tell if it is a drag source (I think) by way of the
protected state() method - there might be some way of leveraging this.
   I don't think it's possible to detect state() changes without
polling, though.  Definitely a lot more thought required, here.

I've also found another bug: if we have a split pane with icon view,
then auto-entering a folder in the left pane also causes the right
pane to be auto-entered.  Amazing how tricky something so
simple-sounding can turn out to be! :) My initial thought was maybe to
be able to provide an option originatingView in the triggerItem signal
(NULL by default) as a "hint" as to which view to expand.  Can you
think of a better solution for this?


> [...]
>> 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.
>
> Please just take all the time you need, there is no need to do something
> within 24 hours (except if your name is Jack Bauer :-)

Hehe!

>> 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.
>
> I understand, but exactly this difference should be kept out of the expander
> class itself from my point view. The client of the expander decides   
>  whether it
> should do an expanding or not. If this is based on reading the    
> general setting
> from Dolphin or because the client says "all tree views should be expanded"
> should not be decided inside the expander class.

Hmmm ... Ok, but I'm a bit confused as to how this would solve the
following problem:

- Column view should not auto expand by default; thus, when the user
opens column view, dragging a file over one of directories in the view
will not auto-open it (fine so far).  setEnabled(false) at the time of
creation of the column view will take care of this.

- The user then changes this setting (I'm assuming it will ultimately
be settable from the UI ... ?), so now directories in the column view
need to auto-open on drag hover.  The column view still has
setEnabled(false) from when it was created, though, so this will not
occur.

If the setting is not intended to be changed at runtime, or if we can
get away with only responding to the change in the setting when a new
view is created, then the setEnabled(...) approach at view creation
will be fine - I've been assuming otherwise, which is why the current
implementation was designed the way it was.  Let me know what you think.


>> 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 :)
>
> Honestly speaking I don't know the difference of Konquerors dirfilter plugin
> and Dolphins general filter functionality (Tools -> Show Filter Bar). If the
> functionality from Dolphins filter bar is sufficient, then we'd just have to
> offer this interface to the Dolphin part and filtering would also work with
> the column view. Are there behavioral differences between the filters from
> Dolphin and the dirfilter plugin?

Good point - will investigate.
>
>> 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!
>
> I had no time for this issue yet. Also the whole code for the    
> tooltips has not
> been done by me, so it's difficult to give any advice (but yes: I also think
> KToolTipItem should get an QObject in this case). Maybe you could get in
> direct contact with Fredrik Höglund, that would be great :-)

Sounds like a good idea :) I was initially a little worried about
making changes as everything seems very flexible (more than Dolphin
seems to require) and kdelibs-ish, as if it might be moved there
eventually.  I'll ask Fredrik whether it's destined for kdelibs or to
be kept Dolphin-only and in either case, whether he had any plans in
mind for implementing the preview.

Best Wishes,
Simon