Make a special dedicated popup for OpenStreetMap data in the Desktop version
ClosedPublic

Authored by sanjibanb on Jul 21 2016, 3:11 PM.

Details

Reviewers
nienhueser
rahn

Diff Detail

Repository
R34 Marble
Lint
Lint Skipped
Unit
Unit Tests Skipped
sanjibanb updated this revision to Diff 5392.Jul 21 2016, 3:11 PM
sanjibanb retitled this revision from to Show a separate popup in the Desktop version, for placemarks containing osm data.
sanjibanb updated this object.
sanjibanb edited the test plan for this revision. (Show Details)
sanjibanb added reviewers: nienhueser, rahn.
sanjibanb set the repository for this revision to R34 Marble.
sanjibanb added a project: Marble.
sanjibanb added a subscriber: Marble.
sanjibanb updated this revision to Diff 5393.Jul 21 2016, 3:39 PM

Add missing "%"

sanjibanb updated this revision to Diff 5394.Jul 21 2016, 3:40 PM

Remove line added by mistake.

sanjibanb updated this revision to Diff 5396.Jul 21 2016, 3:53 PM

Put correct keys for website

sanjibanb updated this revision to Diff 5407.Jul 22 2016, 8:54 AM

Add shop info as well

sanjibanb retitled this revision from Show a separate popup in the Desktop version, for placemarks containing osm data to Make a special dedicated popup for OpenStreetMap data in the Desktop version.Jul 22 2016, 9:20 AM
sanjibanb updated this revision to Diff 5455.Jul 23 2016, 8:14 AM

Use a more elegant solution to hide html elements

sanjibanb updated this revision to Diff 5474.Jul 24 2016, 8:02 AM

Add svg showing the amenity type, on the top-right corner of the popup

nienhueser edited edge metadata.Jul 24 2016, 8:31 AM

Looks good :-)

To make this really shine we need some improvements in the webpopup implementation, me thinks. @rahn Any ideas? Pain points I see currently are

  • the size of the webpopup is often wrong: Too small (does not fit the content well) or even too large (content/size ratio seems low)
  • it feels heavy
  • it involves a lot of automatic map panning
src/lib/marble/MarbleMap.h
424 ↗(On Diff #5474)

Let's provide a const version only for a start:

const StyleBuilder* styleBuilder() const;
src/lib/marble/MarbleWidget.h
251 ↗(On Diff #5474)

const only for a start here as well

const StyleBuilder* styleBuilder() const;
src/lib/marble/MarbleWidgetPopupMenu.cpp
616 ↗(On Diff #5394)

That seems quite a long statement. What about breaking it down like this?

bool hasOsmData = false;
QStringList recognizedTags;
recognizedTags << "name" << "amenity" << "cuisine" << "opening_hours";
recognizedTags << "addr:street" << ...
foreach(const QString &tag, recognizedTags) {
    if (data.containsTagKey(tag)) {
        hasOsmData = true;
        break;
    }
}
src/lib/marble/webpopup/osm.html
77 ↗(On Diff #5394)

Let's go for a clickable link:

<td>Website:</td><td><a href="%website%">%website%</a></td>
sanjibanb updated this revision to Diff 5475.Jul 24 2016, 9:00 AM
sanjibanb edited edge metadata.
sanjibanb marked 4 inline comments as done.

Fix issues from the previous revision.

sanjibanb updated this revision to Diff 5476.Jul 24 2016, 9:02 AM

Follow convention

rahn accepted this revision.Jul 24 2016, 1:23 PM
rahn edited edge metadata.

Let's sort out the remaining tweaking issues in a separate commit :)

This revision is now accepted and ready to land.Jul 24 2016, 1:23 PM
sanjibanb closed this revision.Jul 24 2016, 1:26 PM

Pushed. Commit for remaining tweaking issues coming up soon.