Review Request: Document switcher plugin improvement

Tue Jan 8 22:23:44 UTC 2013

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

A few smaller things in the code that I don't quite like. But more importantly:

This still depends on manual eliding of the path (i.e. your truncatePath function). I think the truncation should be dropped.

For the size of the view you could use QAbstractItemView::sizeHintForRow and sizeHintForColumn so the view gets its preferred size. I've actually experimented a bit with that, this is my current setViewGeometry function (as you can see its not depending on a contentWidth calculated elsewhere):

    const QSize centralSize = window->centralWidget()->size();

    // Maximum size of the view is 3/4th of the central widget (the editor area) so the view does not overlap the
    // mainwindow since that looks awkward.
    const QSize viewMaxSize( centralSize.width() * 3/4, centralSize.height() * 3/4 );

    // The actual view size should be as big as the columns/rows need it, but smaller than the max-size. This means
    // the view will get quite high with many open files but I think thats ok. Otherwise one can easily tweak the 
    // max size to be only 1/2th of the central widget size
    const QSize viewSize( std::min( view->sizeHintForColumn(0) + view->verticalScrollBar()->width(), viewMaxSize.width() ), 
                          std::min( view->sizeHintForRow(0) * view->model()->rowCount(), viewMaxSize.height() ) );

    // Position should be central over the editor area, so map to global from parent of central widget since 
    // the view is positioned in global coords
    QPoint centralWidgetPos = window->mapToGlobal( window->centralWidget()->pos() );
    const int xPos = std::max(0, centralWidgetPos.x() + (centralSize.width()  - viewSize.width()  ) / 2);
    const int yPos = std::max(0, centralWidgetPos.y() + (centralSize.height() - viewSize.height() ) / 2);

    view->setFixedSize(viewSize);
    view->move(xPos, yPos);

In addition I added these three lines to the constructor:

    view->setUniformItemSizes( true );
    view->setTextElideMode( Qt::ElideMiddle );
    view->setHorizontalScrollBarPolicy( Qt::ScrollBarAlwaysOff );

So the horizontal scrollbar is never displayed and the eliding happens in the middle of the items keeping the last directory part intact. The uniform item sizes is merely for optimization of rendering and layout calculating.

Can you try this out please? What do you think about the behaviour?

As you can see the code bases the maximum size on the central widget. I've tried out your initial patch with a very small mainwindow and looked quite awkward to have the popup be larger than the mainwindow. And having the popup visually inside the bounds of the central widget (the editor area) strengthens the connection it has to this area of the mainwindow.

I'm not 100% satisfied with this, with very long paths it can happen that the project name is cut out. A table or treeview might help with that, i.e. make the first column with filename + project name always resize to its contents and the last one resize to fit the view. Then set a fixed size on the view and let Qt figure out the details. It won't look like a single item anymore, but I can live with that if we get some more control over the rendering. Another option would be to actually do this stuff in a delegate, so the delegate would be fetching/calculating the filename, project name and path and ensure that only the path is being elided by qt when painting the whole thing. Thats quite a bit more complicated to implement though I think.

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19153>

    the width of the scrollbar should be possible to find out, no need to hardcode it.

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19155>

    I think the change that merges walkForward/Backward should be done first as a separate commit. And then on top of that adjust the presentation since it touches the walk function. That way you're not mixing refactoring with actual logic changes and people can later more easily follow why certain changes where done.

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19163>

    This is not quite the right fontmetrics, in qt's itemview the delegate has the ultimate control about rendering and also decides which font is being used. But that information is not available.

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19152>

    Please move the using to the top of the file or remove it completely and do a full qualification of the classes.

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19151>

    We usually use const T * const for const pointers to const types and not const * const.

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19157>

    This should be either a multi-line if or without the braces. I'd prefer the former but don't have a strong opinion.

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19158>

    Same thing here, please keep the comments

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19156>

    Please leave the comments intact when you just re-arrange code but don't change it.

    Also please keep multi-line ifs, we're not trying to obfuscate our code ;)

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19160>

    Hmm, why not use 2 variables instead of the pair? I don't see any benefit of using a pair here since its deleted again a few lines forward and makes it less clear what the two parts are actually (hence your comment about first and second)

plugins/documentswitcher/documentswitcherplugin.cpp
<http://git.reviewboard.kde.org/r/107628/#comment19159>

    This ternary operator is quite unreadable IMHO. I'd write it as:

    fileInProjectInfo = projectNameSize < 0 ? qMakePair(path, QString("/"))
                                            : qMakePair(...)

    i.e. keep the if-expression on the same line and align ? and :.

    I'd also either leave out the comment on this line or add it to the one before the code.

- Andreas Pakulat

On Jan. 7, 2013, 8:06 p.m., Jarosław Sierant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107628/
> -----------------------------------------------------------
> 
> (Updated Jan. 7, 2013, 8:06 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Document switcher window width is automatically calculated base on content (auto adjust to longest item). Current screen width is upper bound of the window width.
> 
> Code duplication has been removed (methods walkForward/walkBackward).
> 
> A path to the file on items list can be truncated if it is too long (threshold=80).
> 
> 
> Diffs
> -----
> 
>   plugins/documentswitcher/documentswitcherplugin.h 0924e81 
>   plugins/documentswitcher/documentswitcherplugin.cpp d457a5a 
> 
> Diff: http://git.reviewboard.kde.org/r/107628/diff/
> 
> 
> Testing
> -------
> 
> Only manual tests are done.
> 
> 
> Screenshots
> -----------
> 
> Document Switcher window based on QTableView
>   http://git.reviewboard.kde.org/r/107628/s/990/
> 
> 
> Thanks,
> 
> Jarosław Sierant
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130108/c2b200d1/attachment-0001.html>