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

[Xuetian Weng]

> On Aug. 26, 2012, 5:58 p.m., Frank Reininghaus wrote:
> > 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.

Ah, actually I'm also thinking about something similar as "m_scrollToCurrentItem" once, the only thing that prevent me doing this is that do I want to change the usage of existing function. (Since I'm not familiar with the dolphin code thus I don't want to touch undirect-related things).

Ok, I'll try then.


- Xuetian


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


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/38ef54bc/attachment.htm>