Some countries are invisible on the political map
ClosedPublic

Authored by McPain on Aug 31 2017, 10:08 AM.

Details

Reviewers
rahn
Group Reviewers
Marble
Summary

This is a fix for bug#381984

Diff Detail

Repository
R34 Marble
Lint
Lint Skipped
Unit
Unit Tests Skipped
McPain created this revision.Aug 31 2017, 10:08 AM
rahn added a subscriber: rahn.Aug 31 2017, 6:12 PM

Thanks a lot! I've just tested it and it works nicely indeed (also for the other vector-based maps).

Being curious: Why is the insertMulti needed here? (maybe there should be a comment there - which would be nice in general given the common usage of auto here (which is fine) ).

Does this patch need a backport to any other branches?

rahn added a comment.Aug 31 2017, 6:14 PM

Now the only other similar regression is see is the initial lack of colorization for the atlas map theme. Might be a nice follow-up issue to look into if you feel like it ;)

In D7625#141862, @rahn wrote:

Being curious: Why is the insertMulti needed here? (maybe there should be a comment there - which would be nice in general given the common usage of auto here (which is fine) ).

You are using QHash.
insert() will replace items if any with specified key exists.
insertMulti() will simply add the new one without replacing recently added.

In D7625#141862, @rahn wrote:

Does this patch need a backport to any other branches?

Yes, of course, if it won't be difficult.

In D7625#141864, @rahn wrote:

Now the only other similar regression is see is the initial lack of colorization for the atlas map theme. Might be a nice follow-up issue to look into if you feel like it ;)

Perhaps I'll take a look later.

rahn added a comment.Sep 13 2017, 5:39 AM

Hi!

Thanks for keeping the ball rolling regarding this very important issue.

The reason why this patch hasn't been submitted into marble master and branch yet is this:

I still wonder whether this patch is a fix or a workaround:

Is it intentional that the feature pointers held in the tileList are used as a key multiple times (and therefore require an insertMulti())? Or should they be unique in the first place (and therefore the multiple usage of the key is only the symptom of a bug in a different place)?

This was actually the thought behind my previous question. So if you can help to shed some light on this then this would help a lot - otherwise I'll try to look into this later ...

In D7625#145329, @rahn wrote:

Is it intentional that the feature pointers held in the tileList are used as a key multiple times (and therefore require an insertMulti())? Or should they be unique in the first place (and therefore the multiple usage of the key is only the symptom of a bug in a different place)?

AFAIK, One polygon on the map = one key.
Countries like Russia or Canada have more than one polygon. If keys are unique, you will see a little piece of country, not the whole one.
Same thing with highlighted countries.

rahn added a comment.Sep 13 2017, 6:48 AM

I looked a bit deeper into the code and indeed it sounds sensible that a feature could possibly mapped to multiple items. So unless there are any concerns from other developers I will commit this later today.

rahn accepted this revision.Sep 14 2017, 4:57 PM

Submitted to master.

This revision is now accepted and ready to land.Sep 14 2017, 4:57 PM
rahn added a comment.Sep 14 2017, 5:21 PM

And backported to 17.08 branch. Does this patch require any more action?

Thanks a lot Oleg for this nice contribution.

Oleg, if you feel like it it would be totally awesome if you could look into the other regression: the initial lack of colorization for the atlas map theme. I currently still lack some time to do it myself ... ;-/

McPain added a comment.EditedSep 14 2017, 6:18 PM
In D7625#145828, @rahn wrote:

And backported to 17.08 branch. Does this patch require any more action?

Nothing but indentation (see 223-226 in patched source).
This wrong indentation is side effect for minimizing patch.

Oleg, if you feel like it it would be totally awesome if you could look into the other regression: the initial lack of colorization for the atlas map theme. I currently still lack some time to do it myself ... ;-/

I wil look at this next week unless I have new tasks at work.
PS do you have any submitted bugs related to this issue?

aacid added a subscriber: aacid.Sep 16 2017, 7:51 PM

@rahn please rememeber to close this when you commit things so they don't stay open forever.

aacid closed this revision.Sep 16 2017, 7:51 PM