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

[Peter Penz]

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...

> 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...

> 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:

* Please follow the coding guidelines specified in 
http://techbase.kde.org/Policies/Kdelibs_Coding_Style

* Use the m_ prefix only for member variables, not for methods.

* Since Qt4/KDE4 it is recommended not using bool parameters in constructors. 
E. g. 
  ... = new AutoExpander(view, m_proxyModel, true)
is not as readable as
  ... = new AutoExpander(view, m_proxyModel, 
AutoExpander::AlwaysExpandThisView)
or as
  AutoExpander* expander = new AutoExpander(view, m_proxyModel);
  expander->setAlwaysExpandThisView(true);

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'...).

* I think some cleanups for the members are needed, there are too many members 
for a quite straight forward logic, which make the class quite difficult to 
understand:

m_viewAsTreeView vs. m_isTreeView:
+  m_viewAsTreeView = qobject_cast< QTreeView* >(view);
+  m_isTreeView = (m_viewAsTreeView != 0);

This information can be retrieved any time in any method, there is no need 
remembering this information in members.

The same is valid for the directory model: You have the proxy model already 
and only need to access the directory model within one method -> I'd suggest 
just getting it there locally instead of using a member.

The member m_expandAnyView is not used at all.

Is the member variable m_isValid necessary? Instead I'd suggest not installing 
the event filter if no valid proxy model is provided.

* I think the name of the setting should be improved:

+        <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?

* Use Q_ASSERTs:

+void AutoExpander::m_autoExpandTimeout()
+{
+  // If we are here, then m_isValid is true.

-> adding a Q_ASSERT(m_isValid) is better than the comment :-)

* Why not just moving the code from init() to the constructor?

* Please take care with comments - only comment what's _not_ already explained 
by the code itself. Comments like this don't make sense:

+AutoExpander::~AutoExpander()
+{
+  // Nothing to do, here!
+}

+  m_isValid = false; // Guilty until proven innocent!

+  // All OK, apparently!
+  m_isValid = true;

+    // Cancel the timer.
+    m_autoExpandTriggerTimer->stop();

* Please use a consistent style for checking null-pointers:
+  if (m_proxyModel == 0)
vs.
+  if (!m_proxyModel)

I personally prefer the:
    if (m_proxyModel == 0)
approach, but on most of the other KDE code
    if (!m_proxyModel)
is used. It's really a matter of taste from my point of view, but I think 
within one class it should be used consistently.

I hope this was not too much nitpicking ;-) Thanks in advance, please let me 
know if I can support you.

Best regards,
Peter

>
> Best Wishes,
> Simon
>
> PS - If you think the whole approach sucks and you have a better way,
> please let me know - I won't be offended ;)