Review Request: renaming style and add new style

[Pierre Stirnweiss]

I am not now in a position to do a proper review for the moment. However a
couple of questions:

- what is it you are trying to do with these "other" paragraph styles?
- why inserting them is not done through the kotext's styleManager like the
"standard" paragraph styles and directly into the model?
- playing around with the model's returned data (from 2 different style
lists) should be done with extreme care (inside beginInsert, beginMove,...
and the like). You might get updating problems in the views otherwise

PierreSt


On Wed, May 23, 2012 at 2:35 PM, C. Boemann <cbr at boemann.dk> wrote:

>    This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104962/
>
> As I asked you submit midway through you work i know it's not done, so i'm not going to do a deep review.
>
> However I've added some comments on what i think you should change
>
>
>    plugins/textshape/dialogs/StyleManager.cpp<http://git.reviewboard.kde.org/r/104962/diff/1-2/?file=64558#file64558line311> (Diff
> revisions 1 - 2)
>
> void StyleManager::buttonNewPressed()
>
>    311
>
>         KoParagraphStyle *newStyle = new KoParagraphStyle();
>
>   you new lines are correct, just keep the setName and selectName) from above
>
>
>    plugins/textshape/dialogs/StylesModel.cpp<http://git.reviewboard.kde.org/r/104962/diff/1-2/?file=64562#file64562line109> (Diff
> revisions 1 - 2)
>
> QVariant StylesModel::data(const QModelIndex &index, int role) const
>
>   109
>
>             }
>
> 109
>
>             }
>
>   please follow this by
>
> if (!paragStyle && m_newStyleList.contains(styleId)) {
>    paragStyle = m_newStyleList[styleId];
> }
>
>
>    plugins/textshape/dialogs/StylesModel.cpp<http://git.reviewboard.kde.org/r/104962/diff/1-2/?file=64562#file64562line110> (Diff
> revisions 1 - 2)
>
> QVariant StylesModel::data(const QModelIndex &index, int role) const
>
>    110
>
>             if (id < 0) {
>
>   not needed if you do the above
>
>
>    plugins/textshape/dialogs/StylesModel.cpp<http://git.reviewboard.kde.org/r/104962/diff/1-2/?file=64562#file64562line403> (Diff
> revisions 1 - 2)
>
> void StylesModel::updateName(int styleId)
>
>   400
>
>             if (paragStyle) {
>
> 403
>
>             if (paragStyle || styleId < 0) {
>
>   this change looks wrong. It could cause a crash. better to write in the line above
>
> if (!paragStyle && m_newStyleList.contains(styleId)) {
>    paragStyle = m_newStyleList[styleId];
> }
>
>
>    plugins/textshape/dialogs/StylesModel.cpp<http://git.reviewboard.kde.org/r/104962/diff/1-2/?file=64562#file64562line413> (Diff
> revisions 1 - 2)
>
> QModelIndex StylesModel::firstStyleIndex()
>
>   410
>
>                     // s should be found as the manager and the m_styleList should be in sync
>
> 413
>
>                     // s should be found as the manager and the m_styleList should be in sync
>
>   and again here you need to add something similar
>
>
>    plugins/textshape/dialogs/StylesModel.cpp<http://git.reviewboard.kde.org/r/104962/diff/1-2/?file=64562#file64562line478> (Diff
> revisions 1 - 2)
>
> void StylesModel::addNewParagraphStyle(KoParagraphStyle *style)
>
>    478
>
>     style->setName(i18n("new Style moji"));
>
>   giving it a name is not the responsibility of the stylemodel
>
>
>    plugins/textshape/dialogs/StylesModel.cpp<http://git.reviewboard.kde.org/r/104962/diff/1-2/?file=64562#file64562line479> (Diff
> revisions 1 - 2)
>
> void StylesModel::addNewParagraphStyle(KoParagraphStyle *style)
>
>    479
>
>     style->setStyleId(-100);
>
>   obviously you will need to assign different numbers, so we can insert more than one new style, before applying
>
>
> - C.
>
> On May 23rd, 2012, 12:12 p.m., mojtaba shahi wrote:
>   Review request for Calligra.
> By mojtaba shahi.
>
> *Updated May 23, 2012, 12:12 p.m.*
> Description
>
> a new review for adding new style and renamimg styles
>
>   Diffs
>
>    - plugins/textshape/dialogs/CharacterGeneral.h (eed6a02)
>    - plugins/textshape/dialogs/CharacterGeneral.cpp (c951b27)
>    - plugins/textshape/dialogs/ParagraphGeneral.h (3cf9824)
>    - plugins/textshape/dialogs/ParagraphGeneral.cpp (2fdb272)
>    - plugins/textshape/dialogs/StyleManager.h (44dff97)
>    - plugins/textshape/dialogs/StyleManager.cpp (c318dd7)
>    - plugins/textshape/dialogs/StyleManagerDialog.h (56e36b4)
>    - plugins/textshape/dialogs/StyleManagerDialog.cpp (d423ae0)
>    - plugins/textshape/dialogs/StylesModel.h (53c0225)
>    - plugins/textshape/dialogs/StylesModel.cpp (3b03f1b)
>
> View Diff <http://git.reviewboard.kde.org/r/104962/diff/>
>
> _______________________________________________
> calligra-devel mailing list
> calligra-devel at kde.org
> https://mail.kde.org/mailman/listinfo/calligra-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20120523/9d2f0be5/attachment.htm>