Review Request: Replace the current half baked style selection widgets with a home brewed comboBox

Sun Jan 15 14:05:00 GMT 2012

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > libs/kotext/styles/KoCharacterStyle.cpp, lines 2130-2131
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46456#file46456line2130>
> >
> >     This change seems to be wrong as it will return stuff that should not be returned here.
> 
> Pierre Stirnweiss wrote:
>     Well, these hard coded default are applied when the style is applied. And since this function does not return only what is defined in that particular style but also moves up the parenting chain, i think these properties should also be returned. If not, there is a discrepancy between what the styles apply on a format and what the style returns as a value:
>     If I have:
>     QTextCharFormat format;
>     style->applyStyle(format);
>     Then format.property(some hard coded property) returns a value, but style.value(that same property) is undefined. I don't find this discrepancy logical.

Changed it. Discussed with Boemann who made me see the light ;)

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > libs/textlayout/KoStyleThumbnailer.cpp, line 124
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46461#file46461line124>
> >
> >     The if is useless as it will always a pointer when clone is a pointer.

Actually this can't be done by clone. clone is initially a KoParagraphStyle. When accessing the properties of clone, we get the paragraph properties, not the character properties.
I agree that since a paragraphStyle is an inherited characterStyle, this test should never fail (until next re-factoring, in which case we would crash if we forget about that area of code, which we probably would)

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/CMakeLists.txt, lines 38-39
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46462#file46462line38>
> >
> >     Would it make sense to move the to libs/widgets ?

If these get used somewhere else than TextShape, sure. For now, I have designed them with textShape only in mind. Should we move them, the API might want to be adjusted.

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/TextTool.cpp, lines 1803-1810
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46464#file46464line1803>
> >
> >     How about intializing charStyle with style?

There is no KoCharacterStyle(KoCharacterStyle *otherStyle) constructor. So I don't think we can.

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/TextTool.cpp, lines 2125-2126
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46464#file46464line2125>
> >
> >     Should we remove the properties in the char style that are already there in the paragraph styles. That would make it possible to e.g. only get bold or italic instead of all properties.

I don't know about that one. CBoemann?

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesCombo.h, line 41
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46471#file46471line41>
> >
> >     Please add explicit here.

Arf, forgot about that one. Will fix in next iteration.

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesCombo.cpp, line 26
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46472#file46472line26>
> >
> >     This include seems to be unneeded. Please remove.

idem

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesCombo.cpp, lines 28-29
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46472#file46472line28>
> >
> >     these are not needed.

idem

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesCombo.cpp, lines 31-32
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46472#file46472line31>
> >
> >     These are not needed.

idem

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesCombo.cpp, line 34
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46472#file46472line34>
> >
> >     seems not to be needed.

idem

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesCombo.cpp, lines 106-107
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46472#file46472line106>
> >
> >     There should be no blank after ( and before ). That is true for the hole file.

idem

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesCombo.cpp, line 108
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46472#file46472line108>
> >
> >     the { should be in the line above

idem

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesComboPreview.h, line 25
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46473#file46473line25>
> >
> >     This forward declaration is not used.

idem

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesComboPreview.cpp, line 81
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46474#file46474line81>
> >
> >     I'm not sure if the script to extract the messages does not extract this one.

ok, i'll modify the i18nc with a _ in the middle

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesModel.cpp, line 69
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46478#file46478line69>
> >
> >     I think the As paragraph is not very descriptive of what it does. 
> >     
> >     How about "Style of Paragraph used"?
> >      
> >     Also I think we should translate this string.

Is it ok to burden the translators right now? a translator listening to this perhaps can answer?

> On Jan. 12, 2012, 8:01 a.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/StylesModel.cpp, line 142
> > <http://git.reviewboard.kde.org/r/103677/diff/1/?file=46478#file46478line142>
> >
> >     Same as above about the string

idem

- Pierre

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

On Jan. 15, 2012, 2:03 p.m., Pierre Stirnweiss wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103677/
> -----------------------------------------------------------
> 
> (Updated Jan. 15, 2012, 2:03 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> Merge request.
> Merge branch textshape_stylesWidget_pierreSt into master.
> The branch provides a replacement for the style selection widgets. The new provided widgets are a subclass of QComboBox.
> They provide a style preview both in the main area and in the dropdown menu.
> The dropdown menu provides a "show style manager" button, which call the whole style management dialog.
> The dropdown also provides a "delete style" button, although this is for the moment disabled/commented out, because deleting a style is not currently really supported.
> The combo also allows to create a style based on the current text format if it does not correspond to the original style (ie. the user changed a property).
> 
> 
> Diffs
> -----
> 
>   libs/kotext/KoTextEditor.h 94bf433 
>   libs/kotext/KoTextEditor.cpp 9f242e6 
>   libs/kotext/styles/KoCharacterStyle.h 898ba43 
>   libs/kotext/styles/KoCharacterStyle.cpp f9e0741 
>   libs/kotext/styles/KoParagraphStyle.h 12ab672 
>   libs/textlayout/CMakeLists.txt af6d64a 
>   libs/textlayout/FrameIterator.h c73d580 
>   libs/textlayout/KoStyleThumbnailer.h 8645089 
>   libs/textlayout/KoStyleThumbnailer.cpp 3529bbb 
>   merge_textshape.diff PRE-CREATION 
>   plugins/textshape/CMakeLists.txt fad7c04 
>   plugins/textshape/TextTool.h f8de126 
>   plugins/textshape/TextTool.cpp f6822bd 
>   plugins/textshape/dialogs/SimpleCharacterWidget.h ad944a5 
>   plugins/textshape/dialogs/SimpleCharacterWidget.cpp 6b2bb66 
>   plugins/textshape/dialogs/SimpleCharacterWidget.ui e4f6d3b 
>   plugins/textshape/dialogs/SimpleParagraphWidget.h bc167b4 
>   plugins/textshape/dialogs/SimpleParagraphWidget.cpp cbfacfe 
>   plugins/textshape/dialogs/SimpleParagraphWidget.ui b2e9adf 
>   plugins/textshape/dialogs/StylesCombo.h PRE-CREATION 
>   plugins/textshape/dialogs/StylesCombo.cpp PRE-CREATION 
>   plugins/textshape/dialogs/StylesComboPreview.h PRE-CREATION 
>   plugins/textshape/dialogs/StylesComboPreview.cpp PRE-CREATION 
>   plugins/textshape/dialogs/StylesDelegate.h f5d216f 
>   plugins/textshape/dialogs/StylesDelegate.cpp d1021f5 
>   plugins/textshape/dialogs/StylesModel.h dbd17b4 
>   plugins/textshape/dialogs/StylesModel.cpp 94deb24 
>   plugins/textshape/dialogs/StylesWidget.cpp 4f31a01 
>   plugins/textshape/dialogs/TableOfContentsStyleModel.cpp 48ee7c3 
> 
> Diff: http://git.reviewboard.kde.org/r/103677/diff/diff
> 
> 
> Testing
> -------
> 
> The functionality exclusively linked to the comboBox seems to be working properly. Some defects have been identified (like no paragraph style is selected at startup), but these are actually outside the scope of this widget and affecting the current widget also.
> 
> 
> Thanks,
> 
> Pierre Stirnweiss
> 
>

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