[kde-edu]: Review Request: [Cantor] Improved Script editor window

[Alexander Rieder]

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


Hi,

overall the patch seems good, and the functionality is definately an improvement. so thanks for working on this.
There are still some issues left though:
- dont use KDE/ prefix in your includes, it isn't used anywhere else and this should stay consistent
- use the <kdename.h> includes for kde-headers, e.g. <kxmlguifactory.h> instead of <KXMLGUIFactory> (I may not be completely consistent with this myself, but it should be a guideline)
- The caption correctly updates if I open a file, but it doesn't when saving. You should have a look at your connections, and probably completely remove the parameter, as Qt is smart enough to connect to a slot which takes fewer parameters than the signal.

- Alexander


On 2010-05-04 20:34:30, Miha Cancula wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3878/
> -----------------------------------------------------------
> 
> (Updated 2010-05-04 20:34:30)
> 
> 
> Review request for KDE-Edu and Alexander Rieder.
> 
> 
> Summary
> -------
> 
> Change the script editor to use XmlGui for actions, with complete KTextEditor toolbars and menus.
> 
> The patch is a little rough still, with unneeded whitespace changes, I will probably update it soon. There are also some leftover debug calls and error checks. You can see the code and comment on it. I will also explain and/or correct any wtf-moments that may have occured. 
> 
> I realize that trunk is in a soft treeze right now, and that this isn't a killer feature, so I think there's no rush to get it commited. I would like some feedback on it.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/cantor/src/cantor.cpp 1120620 
>   /trunk/KDE/kdeedu/cantor/src/cantor_part.h 1120620 
>   /trunk/KDE/kdeedu/cantor/src/cantor_part.cpp 1120620 
>   /trunk/KDE/kdeedu/cantor/src/cantor_scripteditor.rc PRE-CREATION 
>   /trunk/KDE/kdeedu/cantor/src/scripteditorwidget.h 1120620 
>   /trunk/KDE/kdeedu/cantor/src/scripteditorwidget.cpp 1120620 
> 
> Diff: http://reviewboard.kde.org/r/3878/diff
> 
> 
> Testing
> -------
> 
> The GUI looks and behaves properly. I tried some different ways of closing the script editor and the running session and it behaved as expected in all of them. 
> 
> It was tested with last week's trunk.
> 
> 
> Screenshots
> -----------
> 
> Script editor with KTextEditor's menus
>   http://reviewboard.kde.org/r/3878/s/385/
> 
> 
> Thanks,
> 
> Miha
> 
>