Review Request 128460: ktp-text-ui: Added a filter for geo uri support

[Alexandr Akulich]

> On July 16, 2016, 12:32 p.m., Sandro Knauß wrote:
> > As I saw inside my browser - marble registers to handle the geo links. As I read somewhere you can also use marble to include a map / so maybe you should connect the marble guys for that ( but this is only an improvement) nothing that holds back this review.

I thought about marble too, but Image and Video messages would have much bigger priority for me.


> On July 16, 2016, 12:32 p.m., Sandro Knauß wrote:
> > filters/geopoint/CMakeLists.txt, line 4
> > <https://git.reviewboard.kde.org/r/128460/diff/2/?file=471820#file471820line4>
> >
> >     use same indent format in a file either a static indent or indent with (

I'm agree with the issue, but I copied this from "highlight" filter. For some (unknown for me) reason we have such indent in all other filters, so I just followed the style.
Do we have some CMake coding convension somewhere?

I changed the style to the static four spaces indentation.


> On July 16, 2016, 12:32 p.m., Sandro Knauß wrote:
> > filters/geopoint/geopoint-filter.cpp, line 62
> > <https://git.reviewboard.kde.org/r/128460/diff/2/?file=471822#file471822line62>
> >
> >     is this compiling with the ; ?

Yes. Actually, it does *not* compiles without the colon. We have this in all filters.


> On July 16, 2016, 12:32 p.m., Sandro Knauß wrote:
> > filters/geopoint/geopoint-filter.cpp, line 42
> > <https://git.reviewboard.kde.org/r/128460/diff/2/?file=471822#file471822line42>
> >
> >     osm using this improved geo link style (if you use the share button at the right panel at openstreetmap.org):
> >     
> >     geo:50.8295,12.9225?z=16
> >     
> >     so add at the end to match the zoom property:
> >     
> >     (?z=(?<zoom>\d+))?
> >     
> >     and replace all [0-9] with \d or is there any need to mix [0-9] and \d ?
> >     
> >     for sure you need to add the logic to set the zoom level to the iframe, too.

I thought to do it later, but ok, just done.


> On July 16, 2016, 12:32 p.m., Sandro Knauß wrote:
> > filters/geopoint/geopoint-filter.cpp, line 38
> > <https://git.reviewboard.kde.org/r/128460/diff/2/?file=471822#file471822line38>
> >
> >     <a href="geo:%1,%2?z=%3">geo:%1,%2?z=%3</a><iframe>...</iframe>
> >     
> >     a user still can see the geo link, that was sent. and can open/copy it like he wants.

Good idea to add a href, but, as I see (just tried it), it would be better to replace a geo text with geo link and add the iframe(s) to the end of message, rather than add href to the end.


- Alexandr


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


On July 16, 2016, 7:04 a.m., Alexandr Akulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128460/
> -----------------------------------------------------------
> 
> (Updated July 16, 2016, 7:04 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-text-ui
> 
> 
> Description
> -------
> 
> Added a geopoint filter which adds an iframe with a map for each geo:<double,double> in message (RFC 5870).
> Uses OpenStreetMap as "backend".
> 
> 
> Diffs
> -----
> 
>   filters/CMakeLists.txt 8118b13 
>   filters/geopoint/CMakeLists.txt PRE-CREATION 
>   filters/geopoint/geopoint-filter.h PRE-CREATION 
>   filters/geopoint/geopoint-filter.cpp PRE-CREATION 
>   filters/geopoint/ktptextui_message_filter_geopoint.desktop.cmake PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/128460/diff/
> 
> 
> Testing
> -------
> 
> It works for me.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20160806/21e1cbd7/attachment.html>