[Marble-devel] Review Request 114729: Improve support for text rendering of the BalloonStyle

[Dennis Nienhüser]

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


Can you include the newly created files in the diff in the future? Then it is possible to add notes to lines directly. If you use git diff to create diffs, you'll have to use git add to have files become of the diff (or work with branches and create commits).

for GeoDataSnippet.h:
Can you change the constructor from
explicit GeoDataSnippet( const QString &text , const int maxLines );
to
explicit GeoDataSnippet( const QString &text=QString() , const int maxLines=0 );

int maxLines();
should be
int maxLines() const;

same for
QString text();
should be
QString text() const;

Both QString m_text and int m_maxLines should be in the private: section.

Can you take a look at the #includes of the file and clean them up? Many of them shouldn't be needed.



src/lib/marble/geodata/data/GeoDataFeature.h
<https://git.reviewboard.kde.org/r/114729/#comment33114>

    Please remove the "If a snippet... not supported" part of the documentation: It's not implemented here like that, and shouldn't be as well.
    
    Doing that might be nice further down when setting the content of the popup though. Maybe add a TODO comment there asking to implement that later on?
    



src/lib/marble/geodata/data/GeoDataFeature_p.h
<https://git.reviewboard.kde.org/r/114729/#comment33115>

    GeoDataSnippet should have a default constructor that does this (ideally setting maxLines=0 meaning no restriction is set) 



src/lib/marble/layers/PopupLayer.cpp
<https://git.reviewboard.kde.org/r/114729/#comment33116>

    Here would be the right place to calculate a snippet implementing the behavior as noticed in the KML reference (see comment above), so I'd insert a TODO comment here.
    


- Dennis Nienhüser


On Dec. 30, 2013, 9:22 a.m., Levente Kurusa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114729/
> -----------------------------------------------------------
> 
> (Updated Dec. 30, 2013, 9:22 a.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This is a GCI task.
> 
> Added handling of $[*] and the displayMode tag.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/layers/PopupLayer.cpp ff554d1 
>   src/lib/marble/layers/PopupLayer.h 06c73c7 
>   src/lib/marble/geodata/data/GeoDataFeature_p.h 92de8a1 
>   src/lib/marble/geodata/data/GeoDataFeature.cpp ba7d952 
>   src/lib/marble/geodata/data/GeoDataFeature.h b54e3e5 
>   src/lib/marble/geodata/CMakeLists.txt 0386159 
>   src/lib/marble/MarbleWidgetPopupMenu.cpp d6fb4a6 
> 
> Diff: https://git.reviewboard.kde.org/r/114729/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> screenie
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/12/29/b71132cb-f7b5-4c83-804c-82e0256fa00b__snapshot2.png
> KML used in screenie
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/12/29/876f1585-3f6b-41a8-81bd-f8f883e00357__baloon.kml
> KmlSnippetTagHandler
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/12/30/4a85ae32-6088-4725-b38b-9f5565473e70__KmlSnippetTagHandler.cpp
> KmlSnippetTagHandler.h
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/12/30/7651a627-ed5e-4ae3-8939-9bb2bc793101__KmlSnippetTagHandler.h
> GeoDataSnippet.cpp
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/12/30/5c00a6a8-117c-482b-b01f-61b1f78a23ba__GeoDataSnippet.cpp
> GeoDataSnippet.h
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/12/30/06dbcca0-2647-45dc-9f70-b7797ed542d5__GeoDataSnippet.h
> 
> 
> Thanks,
> 
> Levente Kurusa
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20131230/9f46ed88/attachment.html>