Implementation of the script manager:
- user can add .js files to a special folder in zanshin
- keep the files in a list
- script execution in zanshin
Details
Details
Upload helloword.js in zanshin, execute the script if the widget "Hello word" pops up it works
helloword.js:
button.text = 'Hello World!'; button.styleSheet = 'font-style: italic; color: green'; button.show();
Diff Detail
Diff Detail
- Repository
- R4 Zanshin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
OK, I stopped reviewing mid-way...
Please pay attention to what I wrote in the other reviews you posted... some lines in this patch I already reviewed in another patch and they're still wrong. Also as I mentioned in a previous review request: don't create new requests each time your code change, update existing requests instead. Otherwise you get me to review again and again the same stuff starting from scratch context wise, it's a poor and difficult way for me to do the reviews. I don't have much time to do those, please make that more efficient. Thanks in advance.
| src/scripting/script.cpp | ||
|---|---|---|
| 28 | Break the line before the : | |
| src/scripting/script.h | ||
| 1 | First the copyright notice, then the include guards. | |
| 33 | Needs to be in the Scripting namespace | |
| 35 | Add the Q_PROPERTY macros for enabled and filename Also provide the NOTIFY part (and then the signals) for those properties. | |
| 42 | isEnabled | |
| 50 | Remove that line | |
| src/scripting/scripthandler.cpp | ||
| 32 | Remove this include | |
| 45 | Remove the "this->" please | |
| 50 | Remove this line please. | |
| src/scripting/scripthandler.h | ||
| 29 | I still don't think QPointer is wanted here. Parenting m_engine like you did should be enough, then returning a QJSEngine* from engine() should be enough too. I think you're trying to workaround an ownership issue you got instead of truely fixing it. | |
| 33 | Please keep the namespace... | |
| 40 | Why did you remove the dependency to TaskRepository? That will be needed as soon as you will expose non trivial functions. | |
| 45 | const reference, although I suspect a raw pointer would make more sense in that case, the execution is synchronous and evaluateScript won't take ownership of the script. | |
| 55 | This is a non-GUI class, depending on QPushButton here makes no sense. | |
| src/scripting/scriptmanager.cpp | ||
| 27 | Remove | |
| 72 | Remove | |
| 74 | And scriptsChanged wouldn't be emitted here?? | |
| src/scripting/scriptmanager.h | ||
| 26 | Include guard convention. | |
| 40 | Really it does too much IMO. Having it just be a container of scripts with signals would be enough. | |
| src/widgets/availablepagesview.cpp | ||
| 136 | Like I pointed out in the other review, this should go to ApplicationComponents. | |
| src/widgets/scripteditor.h | ||
| 25 | Seriously you change for the sake of change and in the wrong way. | |
| 54 | NO, I pointed that out in the other review. | |