Open a new scripting manager dialog under Go/Script Editor
base script manager specifications - wip
Authors: Cedric Bonhomme :: Yann Cetois
Details
Details
- Reviewers
cedricbonhomme - Group Reviewers
Zanshin
Compile, interface only, tests to be implemented once the base script manager is done
Diff Detail
Diff Detail
- Repository
- R4 Zanshin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Sorry guys for the time to finally review it. I was very busy, on the road... Finally got to it now.
A first round of feedback already in the comments. Also note that I think it should be split among several reviews that one, it's doing many things at the same time and introduce two unused classes with no indication of how they'll fit in the rest of the design. And here by indication I don't mean empty comments but other pieces of code starting to use them, like unit tests.
I'd say in the end it'll end up with at least three separate commits: one for the new ui for the ScriptEditor, one for introducing this Script class with at least minimal feature (instead of no feature like now) and one for introducing ScriptManager with at least minimal feature. It'll help much more to figure out where you want to go with this.
| src/scripting/script.cpp | ||
|---|---|---|
| 4 | Split the line before : | |
| src/scripting/script.h | ||
| 1 | Missing copyright notice. Also please change the include guards to be SCRIPTING_SCRIPT_H to match the other classes in the other modules. | |
| 12 | Please remove this comment or make it more informative. | |
| 17 | parent should always be the last parameter. Also don't use 0 for pointers, use Q_NULLPTR instead. Also we tend to use a Ptr typedef for the QSharedPointer types. Also QSharedPointer needs to be passed by const reference. | |
| 22–24 | Remove since there's no signals or public slots. | |
| src/scripting/scripthandler.h | ||
| 51 | Why the change to QPointer? Doesn't seem warranted to me. The ownership currently lies with ScriptHandler so its lifetime is supposedly known. | |
| src/scripting/scriptmanager.h | ||
| 1 | Copyright notice + change for the include guard conventions. | |
| 12 | I smell a god object in the future, if your description is right (it's far reaching into the future for now seeing the implementation) we'll end up with a class having too many responsibilities at once. | |
| 21–22 | No point in that, you probably call setEnabled on the script directly imo. | |
| src/widgets/availablepagesview.cpp | ||
| 136 | This action would make more sense in ApplicationComponents. It's not really related to the list of pages IMO. | |
| src/widgets/scripteditor.cpp | ||
| 1–24 | Again, that's considered disrespectful in the free software world. | |
| src/widgets/scripteditor.h | ||
| 1–26 | Please have more respect for the copyright of the work from others. | |
| 27 | Please don't use connect by convention for slots. I know Qt has this magic but it's evil. :-) | |