Review Request 123352: Fix DockerStylesComboModel

[Thorsten Zachmann]

> On April 15, 2015, 11:34 p.m., Friedrich W. H. Kossebau wrote:
> > plugins/textshape/dialogs/DockerStylesComboModel.cpp, line 192
> > <https://git.reviewboard.kde.org/r/123352/diff/1/?file=361016#file361016line192>
> >
> >     The old code allowed the NoneStyle only as the first item in the source model. This code is okay in whatever position it is passed.
> >     So we trust here the source model to pass the NoneStyle as first, or if not, that any other position also makes sense?
> >     Worth at least a comment, perhaps.

I added a comment that states that the source model already is sorted as wanted.


> On April 15, 2015, 11:34 p.m., Friedrich W. H. Kossebau wrote:
> > plugins/textshape/dialogs/DockerStylesComboModel.cpp, lines 197-198
> > <https://git.reviewboard.kde.org/r/123352/diff/1/?file=361016#file361016line197>
> >
> >     Why no longer explicitely sort by names? This change is not mentioned in the patch description, please tell the motivation for this change. Can proper sorting be assumed to exist in the source model? I could not find this promise in the API dox of `StylesModel`, so please help me why this change is okay :)

You are right it is not needed to sort the model as the StylesModel already has the correct order. Should I add a comment to the StylesModel about that?


- Thorsten


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123352/#review79010
-----------------------------------------------------------


On April 13, 2015, 2:01 p.m., Thorsten Zachmann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123352/
> -----------------------------------------------------------
> 
> (Updated April 13, 2015, 2:01 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> This patch fixes the style combo box showing the wrong name for a style after adding a style.
> 
> To reproduce the problem:
> 
> Create a new stage document (Template Screen Empty):
> Add a text shape
> Write one line of text
> Assign the text the paragraph style Standard.
> Change to center alignment.
> Use the plus button in the paragraph style to assign it the new name S2.
> Change the alignment to right.
> Use the plus button in the paragraph style to assign it the new name S1.
> 
> Now open the combobox and scroll to the top. It will show 3 times S1.
> The problem is that the index in the StylesModel is changed but not index in the DockerStylesComboModel.cpp mapping.
> Therefor all 3 styles pointing to the same index.
> 
> As the model is recreated all the time it just also newly creates the mapping simplifying the code in doing so.
> It also changes the magic -1 to be a Constant.
> 
> 
> Diffs
> -----
> 
>   plugins/textshape/dialogs/DockerStylesComboModel.cpp 409a9f1 
>   plugins/textshape/dialogs/StylesModel.h d15651f 
>   plugins/textshape/dialogs/StylesModel.cpp f996824 
> 
> Diff: https://git.reviewboard.kde.org/r/123352/diff/
> 
> 
> Testing
> -------
> 
> Tested adding of various style with different names. Worked quite nicly
> 
> 
> Thanks,
> 
> Thorsten Zachmann
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20150416/a1974db7/attachment.htm>