This patch passes the previously missing project pointer to the saveState() method, so the QTreeView really does get
saved.
Details
Details
Diff Detail
Diff Detail
- Repository
- R33 KDevPlatform
- Branch
- fix/project-tree-save-state
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
- Check for nullptr (should never happen, but check nevertheless for compatibility with external plugins)
Comment Actions
Style: Please nullptr == foo -> !foo. This is the style we use throughout the KDevelop code base
| plugins/projectmanagerview/projecttreeview.cpp | ||
|---|---|---|
| 439 | I'm not entirely sure what this code is supposed to do. Why do you use m_ctxProject in here, a variable which is only filled when you request the context menu in the project tree view? Please add a comment in which scenario the call to restoreState( m_ctxProject ); actually tries to accommodate. | |
| 446 | ... dito | |
| 470 | Style: p -> project please | |
Comment Actions
Thanks for the review.
I addressed the code style issues and also refactored the plugin a little to get rid of the ugly m_ctxProject altogether. There is no weird internal flip-flop pointer anymore which keeps track of a previous selection. Instead, selections are now queried on demand only.
I think the code flow should be much clearer with this, also in the existing parts. It also makes sure that everything works even when the treeview is manipulated by means other than a right-click menu.
Comment Actions
- Save previous selection, but only use it if nothing else is selected at the moment
I noticed that completely removing the m_ctxProject pointer breaks my previous change that restores the treeview state also
after reconfiguring the project. This is because the selection isn't set in time after closing the project settings.
Therefore, I added a pointer again which saves the previous selection, but it is only used in one method and only when
nothing else is selected at the moment (which appears to me as a much cleaner solution than what we had before my patch).
Comment Actions
This makes a lot more sense with the m_ctxProject stuff gone. :-)
One meaningful comment above, plus a couple of trivial style complaints.
| plugins/projectmanagerview/projecttreeview.cpp | ||
|---|---|---|
| 307 ↗ | (On Diff #11542) | 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). |
| 391 ↗ | (On Diff #11542) | (style again) if ( auto project = getCurrentProject() ) { ? Also lines 298/299. |
| 431 ↗ | (On Diff #11542) | More nitpicking - kdevplatform uniformly uses const auto& for this (with the same meaning). Some more instances below. |
| plugins/projectmanagerview/projecttreeview.h | ||
| 66 ↗ | (On Diff #11542) | Trivial style issue - the existing methods don't have spaces here. |
| plugins/projectmanagerview/projecttreeview.cpp | ||
|---|---|---|
| 307 ↗ | (On Diff #11542) | This is needed to restore the tree state after opening the project (CMake) settings and reconfiguring the project. |
| plugins/projectmanagerview/projecttreeview.h | ||
| 66 ↗ | (On Diff #11542) | Well, some have, some don't. But I'll change it. It's not my own code style anyway, always need to remind myself to add spaces. |
Comment Actions
- Fix more code style issues, and also make the whole thing work when there is only one (unselected) project
I updated the code according to your requests. I don't like having assignment operators inside if statements, personally, but you're the boss. ;-)
I also made sure that it works when there is only one project and it isn't selected. In that case I simply select the entry automatically and proceed as usual (since this happens before or right after a project is added to or removed from the tree view, there is no difference in behavior from a user perspective).
I also added a projectClosed slot to reset the previous selection explicitly, because it appears that using a QPointer alone here isn't enough. Some resources are freed mid-air while tearing down everything when KDevelop quits. For it to work
automagically, I would need to make a QPointer of ProjectBaseItem and not only of ProjectBaseItem::project(), but since ProjectBaseItem is no child of QObject, this isn't possible. If you have a better solution, please speak.
| plugins/projectmanagerview/projecttreeview.cpp | ||
|---|---|---|
| 297 | Where do you want me to remove the spaces inside brackets? Only in if statements or everywhere? I was trying to match the coding style of the file. It isn't very consistent, but one thing that was done (almost) everywhere was adding spaces inside brackets. | |
| plugins/projectmanagerview/projecttreeview.cpp | ||
|---|---|---|
| 297 | ugh, ignore this then (we have so many style inconsistencies... :( | |
| plugins/projectmanagerview/projecttreeview.cpp | ||
|---|---|---|
| 297 | Alright. I only removed the get prefix then. | |
Comment Actions
There's not really any point nagging about the style further, when the existing document is totally inconsistent already.
The actual changes look ok to me, and presumably to kfunk if he's also down to whitespace issues... go ahead ;-)
Comment Actions
+1, I only looked at the visible context shown by phabricator, and there no spaces where added. We really need to run clang-format on kdev* and introduce a hook to ensure no new violations of the style take place...
Comment Actions
assuming you don't have commit rights, what is your email address so I can use that to attribute this patch back to you?
Comment Actions
Ah, since you used arc, it does preserve it. Otherwise not: https://phabricator.kde.org/T5242
I'll land this patch now with the original author mail you used in the commit.
Thanks a lot!
Comment Actions
@bevendorff Your patch seems to cause a bug in KDevelop: https://bugs.kde.org/show_bug.cgi?id=379433 -- care to have a look and fixup your patch?