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

Thorsten Zachmann t.zachmann at zagge.de
Thu Jan 12 08:01:27 GMT 2012 

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


I compiled the patch. However the visual appearance of the selector is not really nice. 
See http://www.zagge.de/files/styleselector.jpg on how it looks.
There is a yellow background behind the text and when you open the selector a lot of triangles are painted.
Also the fonts used to show the text is quite small. I suspect the non set font in the style to be the issue.

If I have a char style that only defines bold it also changes the font when I apply the style. However this bug was also there before and is not related to the patch.


libs/kotext/styles/KoCharacterStyle.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8063>

    This change seems to be wrong as it will return stuff that should not be returned here.



libs/textlayout/KoStyleThumbnailer.h
<http://git.reviewboard.kde.org/r/103677/#comment8065>

    I would prefer if we could change the parameter dirtyCache to recreateThumbnail or updateThumbnail or updateCache as it makes more clear what it does.



libs/textlayout/KoStyleThumbnailer.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8068>

    Line 1,2 and 4 can be moved to the initialisation list of the Private constructor and line 3 to the contructor of the PRivate



libs/textlayout/KoStyleThumbnailer.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8069>

    I think these 2 should be deleted in the destructor of the Private.



libs/textlayout/KoStyleThumbnailer.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8071>

    you should first check if !dirtyCache is set before doing the more expensive operation of getting the object out of the cache. It would also make sense to only call the function object once and store the value. There is no need to call the same function twice.



libs/textlayout/KoStyleThumbnailer.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8073>

    The same can be done by
    QTextFormat format;
    clone->KoCharStyle::applyStyle(format);
    
    But is it really needed doesn't the applyStyle of the close already set this?



libs/textlayout/KoStyleThumbnailer.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8072>

    The if is useless as it will always a pointer when clone is a pointer.



libs/textlayout/KoStyleThumbnailer.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8074>

    Do we really need both methods? The second one could have the QSize as last parameter and pass empty one as default.



libs/textlayout/KoStyleThumbnailer.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8075>

    you should first check if !dirtyCache is set before doing the more expensive operation of getting the object out of the cache. It would also make sense to only call the function object once and store the value. There is no need to call the same function twice.



plugins/textshape/CMakeLists.txt
<http://git.reviewboard.kde.org/r/103677/#comment8076>

    Would it make sense to move the to libs/widgets ?



plugins/textshape/TextTool.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8077>

    How about intializing charStyle with style?



plugins/textshape/TextTool.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8078>

    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.



plugins/textshape/dialogs/SimpleParagraphWidget.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8079>

    Please move to the initialisation list of constructor.



plugins/textshape/dialogs/StylesCombo.h
<http://git.reviewboard.kde.org/r/103677/#comment8087>

    Please add explicit here.



plugins/textshape/dialogs/StylesCombo.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8080>

    This include seems to be unneeded. Please remove.



plugins/textshape/dialogs/StylesCombo.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8083>

    these are not needed.



plugins/textshape/dialogs/StylesCombo.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8084>

    These are not needed.



plugins/textshape/dialogs/StylesCombo.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8082>

    seems not to be needed.



plugins/textshape/dialogs/StylesCombo.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8086>

    There should be no blank after ( and before ). That is true for the hole file.



plugins/textshape/dialogs/StylesCombo.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8085>

    the { should be in the line above



plugins/textshape/dialogs/StylesComboPreview.h
<http://git.reviewboard.kde.org/r/103677/#comment8088>

    This forward declaration is not used.



plugins/textshape/dialogs/StylesComboPreview.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8089>

    I'm not sure if the script to extract the messages does not extract this one.



plugins/textshape/dialogs/StylesModel.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8090>

    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.



plugins/textshape/dialogs/StylesModel.cpp
<http://git.reviewboard.kde.org/r/103677/#comment8091>

    Same as above about the string


- Thorsten Zachmann


On Jan. 11, 2012, 11:11 p.m., Pierre Stirnweiss wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103677/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2012, 11:11 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.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 
>   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/20120112/ba770610/attachment.htm>

More information about the calligra-devel mailing list