Review Request: ProjectSelection class to select one of the opened projects

Sun Jan 24 02:29:25 UTC 2010

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

(Updated 2010-01-24 02:29:25.361850)

Review request for KDevelop.

Changes
-------

I have updated the patch to the new location of veritas library in playground.

As I got no responses and now veritas library is in playground, I have taken these decisions about the questions asked:
-I'll put ProjectSelection class in veritas library and link coverage plugin against veritas library. The other option would be duplicating the code of ProjectSelection class in the coverage plugin, which is even worse than having to link to veritas.
-D-pointers are used for ProjectSelection class, as all the other public classes in veritas use them.
-In ProjectSelection constructor it is checked whether there is a Core or not. Although this is modifying the code to make testing more comfortable, the sacrifice is small compared to the gain.
-ProjectSelectionFilter is called now IProjectSelectionFilter. Although it is not a pure interface, I have seen other abstract base classes prefixed with I, so I'll follow that nomenclature. Also, it is kept as a nested class of ProjectSelection, as it is a class that doesn't have a lot of meaning without its container class.

I'll commit it in about a week from now if there are no objections in that time.

Summary
-------

I have refactored the KSelectAction used in xTest plugin to select a project and extracted its functionality (plus some features not present in the original code) to a class named ProjectSelection, which is a KSelectAction that contains an item for each opened project. I've refactored KDevPlatform xTest plugin to use that class, and I'll also use it in KDevelop Coverage plugin (that's why I extracted the functionality to a self contained class).

However, I have some questions about KDevPlatform code organization and style.

First of all, where should I put the new ProjectSelection class? In this patch I put it in "kdevplatform/veritas" just because I extracted the class from veritas code, but it doesn't really depend on veritas framework. In fact, KDevelop Coverage plugin (the one I'm going to use it in) is no longer part of veritas/xUnit and it doesn't even link to it. So, is there a more suitable place for that class? Also please note that both veritas/xTest and coverage plugins were moved to playground.

Next, should I use d-pointers for ProjectSelection class? Right now it has several private attributes, but I don't know if there is any ABI compatibility restriction to follow.

Internally, ProjectSelection connects to IProjectController signals, and it gets it from ICore. If there is no Core, it doesn't work (and, in fact, it crashes). In normal use there will always be a Core (or I think so), but in unit tests this forces the tests to initialize a Shell (AutoTestShell) and Core whenever a ProjectSelection is created.

This is inevitable in ProjectSelection test itself, but can be annoying when testing something that creates a ProjectSelection (like veritas framework), as it shows a main window which isn't used for anything. So I made a check in ProjectSelection constructor to not to connect any slot if there is no Core. However, I'm not really satisfied with this approach, as this is modifying the code to make tests more comfortable. So, should I keep the check or remove it and fix the tests (causing the annoyance of the unneeded main window)? Is there any other solution?

Finally, a ProjectSelection can filter which projects are enabled to be selected and which not using a ProjectFilter subclass object. ProjectFilter is an abstract base class to be extended with concrete filter behavior. Should it be renamed to IProjectFilter (although it is not a pure interface)? Moreover, right now it is an internal class of ProjectSelection. Should I move it out? If yes, to its own file or in projectselection.h itself but out of ProjectSelection class?

Thank you for your attention.

Diffs (updated)
-----

  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/CMakeLists.txt 1079294 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/internal/runnerwindow.h 1079294 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/internal/runnerwindow.cpp 1079294 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/projectselection.h PRE-CREATION 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/projectselection.cpp PRE-CREATION 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/testrunner.h 1079294 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/testrunner.cpp 1079294 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/tests/CMakeLists.txt 1079294 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/tests/projectselectiontest.h PRE-CREATION 
  /trunk/playground/devtools/kdevelop4-extra-libraries/veritas/tests/projectselectiontest.cpp PRE-CREATION 

Diff: http://reviewboard.kde.org/r/1534/diff

Testing
-------

Thanks,

Daniel