[Differential] [Commented On] D4665: Fix project tree state not being saved

[Francis Herne]

flherne added a comment.


  This makes a lot more sense with the m_ctxProject stuff gone. :-)
  
  One meaningful comment above, plus a couple of trivial style complaints.

INLINE COMMENTS

> projecttreeview.cpp:307
>  
> -    if( !itemlist.isEmpty() )
> -    {
> -        m_ctxProject = itemlist.at(0)->project();
> -    } else
> -    {
> -        m_ctxProject = nullptr;
> +    // add previous selection if nothing is selected right now
> +    if ( itemlist.isEmpty() && m_previousSelection ) {

Is this a good idea? It seems possibly confusing if the user doesn't remember which project was selected before.

If there's only one project, that should be assumed to be selected (cf. https://phabricator.kde.org/D4216).

> projecttreeview.cpp:391
> +    IProject* project = getCurrentProject();
> +    if ( project ) {
>          IProjectController* ip = ICore::self()->projectController();

(style again) `if ( auto project = getCurrentProject() ) {`  ?

Also lines 298/299.

> projecttreeview.cpp:431
> +    if ( !parent.model() ) {
> +        for ( auto const& project: getSelectedProjects() ) {
> +            restoreState( project->project() );

More nitpicking - kdevplatform uniformly uses `const auto&` for this (with the same meaning).

Some more instances below.

> projecttreeview.h:66
>          void aboutToShutdown();
> +        void rowsInserted( const QModelIndex& parent, int start, int end ) override;
> +        void rowsAboutToBeRemoved( const QModelIndex& parent, int start, int end ) override;

Trivial style issue - the existing methods don't have spaces here.

REPOSITORY
  R33 KDevPlatform

REVISION DETAIL
  https://phabricator.kde.org/D4665

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: bevendorff, mwolff, apol, kfunk
Cc: flherne, kfunk, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170220/a8d6266a/attachment-0001.html>