Review Request: Dolphin: make --select scroll to the item need to be selected

[Frank Reininghaus]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106209/#review18041
-----------------------------------------------------------


Thanks for the new patch!

I noticed that there is a small problem which I overlooked. When your patch is applied and loading the folder is finished, such that the file item model emits directoryLoadingCompleted(), two things happen:

1. DolphinView::selectAndScrollToCreatedItem() is invoked, makes the first URL passed on the command line after "--select" the current item and selects this item.
2. DolphinView::slotDirectoryLoadingCompleted() is invoked and calls DolphinView::updateViewState() via a single-shot timer. This function then selects *all* URLs passed on the command line after "--select".

This results in a small, but noticeable delay between the selection of the first URL and the other URLs after "--select".

Therefore, I'm wondering if it might be better to change the code around m_currentItemUrl and m_createdItemUrl more radically, because I think that having the directoryLoadingCompleted() signal invoke two different slots in DolphinView, which do different and partially competing things, is not optimal (which is not your fault, of course, this issue is already in the existing code). How about this:

a) Add a new bool m_scrollToCurrentItem (best put just after m_currentItemUrl), which defaults to false.
b) In markUrlAsCurrent(), set m_currentItemUrl to "url" and make m_scrollToCurrentItem true.

c) In DolphinView::updateViewState(), just after selectionManager->setCurrentItem(currentIndex), scroll to the current item if m_scrollToCurrentItem is true and set it to false then.
d) In DolphinView::observeCreatedItem(), just do the following:

markUrlAsCurrent(url);
markUrlsAsSelected(QList<KUrl>() << url);

e) Remove m_createdItemUrl and DolphinView::selectAndScrollToCreatedItem().

What do you think about this? It's just an idea, and I haven't actually tried it yet, but I think that this should fix the small delay, get rid of the partial code duplication in selectAndScrollToCreatedItem() and updateViewState(), and make the code a bit nicer overall.

Don't get me wrong: I like your patch and I think that it's already a nice improvement compared to the current situation. I just think that the existing code is more complicated than it needs to be, and I that this is a good opportunity to make it better.

- Frank Reininghaus


On Aug. 26, 2012, 12:57 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106209/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2012, 12:57 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> dolphin and konqueror provides "--select" to select items in arguments, but its not very useful since it doesn't scroll to the corresponding item. This patch is supposed to fix this problem.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphinpart.cpp d1626e5 
>   dolphin/src/views/dolphinview.h 1ad4d6c 
>   dolphin/src/views/dolphinview.cpp 233c700 
> 
> Diff: http://git.reviewboard.kde.org/r/106209/diff/
> 
> 
> Testing
> -------
> 
> No problem here and work as expected.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20120826/ba7cb184/attachment.htm>