Fix bug where yellow icon is shown when icon tag is empty #5819 #5821

josh-bernstein · 2017-09-07T19:13:46Z

cesium-concierge · 2017-09-07T19:13:47Z

Welcome to the Cesium community @josh-bernstein!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

josh-bernstein · 2017-09-07T19:23:32Z

My machine's autoformatter made more changes than I meant to

…iumGS#5819" This reverts commit 1fdc621.

josh-bernstein · 2017-09-07T20:06:19Z

I have updated CHANGES.md.

…cified icon are displayed with yellow icon CesiumGS#5819

hpinkos · 2017-09-07T20:07:01Z

Thanks @josh-bernstein! Make sure you send over a CLA so we can review this. We'll probably also want to add a unit test for this particular case.

@tfili do you want to review?

tfili · 2017-09-12T02:06:31Z

Thanks @josh-bernstein. It looks good, but as Hannah mentioned we would want a unit test. It can probably just load a kml chunk and then verify that there isn't an image in the entity.

josh-bernstein · 2017-09-12T15:13:51Z

@tfili Thanks for the response! Can you please tell me where the test file is located?

biomassives · 2017-09-12T15:35:34Z

This message was created automatically by mail delivery software. A message that you sent could not be delivered to one or more of its recipients. This is a temporary error. The following address(es) deferred: [email protected] Domain biomassiv.es has exceeded the max emails per hour (106/100 (106%)) allowed. Message will be reattempted later

…

------- This is a copy of the message, including all the headers. ------ Received: from github-smtp2-ext5.iad.github.net ([192.30.252.196]:40329 helo=github-smtp2a-ext-cp1-prd.iad.github.net) by chi-server32.websitehostserver.net with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from <[email protected]>) id 1drnDl-000huE-C2 for [email protected]; Tue, 12 Sep 2017 10:35:29 -0500 Date: Tue, 12 Sep 2017 08:27:53 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1505230488; bh=e9INILpxZkBGXPEbMcZAhNg+p6SVSbbcwD8PKvYfd+8=; h=From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=Yf8vNlKp/LLCFvCYzjGikkKt87n30/fOLfzi0IV6VhZl6eNcGxYeW5j+n2ohWJYpm xXiGQaeU0Q2NZ3uJfv1TmzLDuNstXl/DJBP/AEGdFeqTh8fuF8qrmCK9IsQWW0EUVn 7DkTMAbqqMHjiW279TuXsLLX0npnhYgVp4rX/MVU= From: Joshua Bernstein <[email protected]> Reply-To: AnalyticalGraphicsInc/cesium <[email protected]> To: AnalyticalGraphicsInc/cesium <[email protected]> Cc: Subscribed <[email protected]> Message-ID: <AnalyticalGraphicsInc/cesium/pull/5821/[email protected]> In-Reply-To: <AnalyticalGraphicsInc/cesium/pull/[email protected]> References: <AnalyticalGraphicsInc/cesium/pull/[email protected]> Subject: Re: [AnalyticalGraphicsInc/cesium] Fix bug where yellow icon is shown when icon tag is empty #5819 (#5821) Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="--==_mimepart_59b7fcf980bbe_84d3f918b015c3c982a5"; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: list X-GitHub-Sender: josh-bernstein X-GitHub-Recipient: biomassives X-GitHub-Reason: subscribed List-ID: AnalyticalGraphicsInc/cesium <cesium.AnalyticalGraphicsInc.github.com> List-Archive: https://github.com/AnalyticalGraphicsInc/cesium List-Post: <mailto:[email protected]> List-Unsubscribe: <mailto:unsub+0042d4e2b706f8e9fd49f38662022d3a0d67c27c7118292392cf0000000115cfbef992a169ce0f42e08d@reply.github.com>, <https://github.com/notifications/unsubscribe/AELU4j0rCpxp5OSYXeH7uYDS9jt2jzERks5shqL5gaJpZM4PQRDa> X-Auto-Response-Suppress: All X-GitHub-Recipient-Address: [email protected]

----==_mimepart_59b7fcf980bbe_84d3f918b015c3c982a5 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit @tfili Thanks for the response! Can you please tell me where the test file is located?

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #5821 (comment) ----==_mimepart_59b7fcf980bbe_84d3f918b015c3c982a5 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit <p><a href="https://github.com/tfili" class="user-mention">@tfili</a> Thanks for the response! Can you please tell me where the test file is located?</p> <p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="#5821 (comment) it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AELU4lOXE_h7wemH6WUY6ucS9tIUW5L_ks5shqL5gaJpZM4PQRDa">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AELU4puZ-NP7q9xjOaqEj44oUJau79b0ks5shqL5gaJpZM4PQRDa.gif" width="1" /></p> <div itemscope itemtype="http://schema.org/EmailMessage"> <div itemprop="action" itemscope itemtype="http://schema.org/ViewAction"> <link itemprop="url" href="#5821 (comment)> <meta itemprop="name" content="View Pull Request"></meta> </div> <meta itemprop="description" content="View this Pull Request on GitHub"></meta> </div> <script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/AnalyticalGraphicsInc/cesium","title":"AnalyticalGraphicsInc/cesium","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/AnalyticalGraphicsInc/cesium"}},"updates":{"snippets":[{"icon":"PERSON","message":"@josh-bernstein in #5821: @tfili Thanks for the response! Can you please tell me where the test file is located?"}],"action":{"name":"View Pull Request","url":"#5821 (comment)> ----==_mimepart_59b7fcf980bbe_84d3f918b015c3c982a5--

tfili · 2017-09-12T15:56:59Z

@josh-bernstein All tests are located in the Specs folder at the same location as in the Source folder. In this case Specs/DataSources/KmlDataSourceSpec.js

This reverts commit ab50ab5.

Merge master into my fork

josh-bernstein · 2017-09-14T21:39:57Z

@hpinkos @tfili I have added unit test. I am still waiting on my company to fill out the CLA.

mramato · 2017-09-27T14:50:07Z

@josh-bernstein thanks again for this PR. Is there any update on the CLA?

josh-bernstein · 2017-09-27T14:53:57Z

@mramato My company says they should be sending it this week!

mramato · 2017-09-27T14:55:28Z

Awesome! Thanks.

pjcozzi · 2017-09-28T21:51:40Z

We now have a CLA from @josh-bernstein, thanks!

@josh-bernstein can you please add yourself to CONTRIBUTORS.md under the corporate CLA section?

tfili · 2017-09-28T22:14:06Z

Source/DataSources/KmlDataSource.js

@@ -1111,10 +1111,6 @@ define([
            entity.billboard = billboard;
        }

-        if (!defined(billboard.image)) {


By doing this, it fixes your case, but breaks the case where there isn't an icon style at all. In that case Google Earth does show a yellow push pin.

Changing the if to something like

if (!defined(styleEntity.billboard) && !defined(billboard.image)) {

will only set the default image if an icon style wasn't defined, which I believe is the correct behavior in both cases.

tfili · 2017-09-28T22:16:16Z

Specs/DataSources/KmlDataSourceSpec.js

+            expect(source.entities);
+            expect(source.entities.values.length).toEqual(1);
+            expect(source.entities._entities._array.length).toEqual(1);
+            expect(!source.entities._entities._array[0]._billboard._image);


You need to expect it to be something. This would probably be toBeUndefined().

Also add a test for the other case where no icon style is specified at all, to verify there is the default push pin.

josh-bernstein · 2017-10-17T19:30:47Z

@tfili When I look at my code changes for CHANGES.md, it does not show anything removed. I did remove my change from the 1.38 version, maybe that's what you're seeing.

Also, my unit tests pass locally. I see that Travis is failing, but I'm not sure what is causing those errors.

tfili · 2017-10-17T20:34:20Z

@josh-bernstein Are you sure you are running all the tests? It fails for me locally and stepping into the code, I don't know how it would pass anywhere.
In the Styles: inline styles take precedance over shared styles test, what is happening is

The <IconStyle> gets processed with the href of http://test.invalid
Then the inline style gets processed. This doesn't have a href. The styles are merged and this is where you have a problem.
- In master since image is undefined in the inline style, the billboard fallsback to the IconStyle.image and is set to http://test.invalid
- In you branch since image is undefined, you set it to the pushpin and that's how it remains, so the test fails because it expects billboard.image to be http://test.invalid but instead it is a canvas element.
We need to push off figuring out what to use till processPositionGraphics once all the potential styles have been merged.
The cases we have are
- CORRECT
  - Style has no IconStyle - Shows yellow pushpin
    - entity.billboard is undefined
  - Style has IconStyle with Icon - Uses the Icon
    - entity.billboard contains image
  - Style has IconStyle with empty Icon - Show nothing
    - entity.billboard is defined but with no image
- INCORRECT
  - Style has IconStyle with no Icon - Shows nothing but should show yellow pushping
    - entity.billboard is defined but with no image

My suggestion would be in processBillboardIcon, that if you don't have a Icon or a color then you don't create a billboard at all. Then it would fallback to the yellow pushpin since entity.billboard would be undefined. This adds an additional case however, which would be no Icon but has a color. You would need to account and write a test for this additional case.

My mistake about CHANGES.md. I guess I was looking at an intermediate change. Looks fine in your branch.

You don't need to close the PR when you are working on it. It won't be merged till all the tests pass and comments are addressed anyway. Just comment again once it is ready.

This reverts commit 470810d.

josh-bernstein · 2017-10-20T17:29:22Z

@tfili Thank you for comment. I believe I have addressed all the concerns you raised. I have once again made significant code changes to KmlDataSource.js, so please re-review code.

josh-bernstein · 2017-10-20T18:01:33Z

Actually I think I'm still failing this test

load works with a KMZ file

EDIT: I thought I saw an error once, but I have since been unable to reproduce.

EDIT: The error I saw once was I timeout error. It was with one of the kmz tests, but I am not positive which one. I have tried to run all tests twice since that and have not been able to reproduce

josh-bernstein · 2017-10-23T14:22:03Z

@tfili I'm not sure why the Travis builds keep failing. When I look at the errors, none look related to my branch. Also, when I run the tests locally with master branch checked out, I get errors. So I do not believe that any of these errors are related to my branch. Therefore, I believe my branch is good to go.

Please let me know if you have thoughts.

tfili · 2017-10-23T15:11:53Z

Source/DataSources/KmlDataSource.js

@@ -729,6 +729,12 @@ define([

        var iconNode = queryFirstNode(node, 'Icon', namespaces.kml);
        var icon = getIconHref(iconNode, dataSource, sourceUri, uriResolver, false, query);
+
+        // If icon tags are present but blank, we do not want to show an icon
+        if (defined(iconNode) && !defined(icon)) {


Reusing the icon variable as a temporary with a different type, even though fine in Javascript, isn't best practice. I would just use a separate variable.

Then you can simplify everything like this

var blankIcon = defined(iconNode) && !defined(icon); ... if (!defined(billboard.image) && !blankIcon) { billboard.image = dataSource._pinBuilder.fromColor(Color.YELLOW, 64); }

blankIcon would be set in the processBillboardIcon method, while the check if (!defined(billboard.image) && !blankIcon) would need to happen in processPositionGraphics method.

Therefore, I believe the only ways to incorporate your suggestions would be to make blankIcon a global variable or a property of billboard. Is either of these ways better than the current way?

tfili · 2017-10-23T15:12:43Z

@josh-bernstein Looks good. I've seen that test fail before. I restarted the build. Hopefully it resolves itself. Other than that I just have the one comment on your new code.

josh-bernstein · 2017-10-23T15:33:31Z

@tfili Please see my reply to your comment

hpinkos · 2017-10-23T15:40:43Z

@josh-bernstein I fixed the failing test in master. If you merge that into your branch, it should pass. Thanks!

josh-bernstein · 2017-10-23T15:45:25Z

Awesome thanks @hpinkos

josh-bernstein · 2017-10-23T15:46:38Z

Actually @hpinkos it appears that master is already merged into my branch

tfili · 2017-10-23T17:47:30Z

@josh-bernstein My mistake.

Also, it looks like you merged master in 4 hours ago, but the fix wasn't merge until 2 hours ago. Could you merge it in so the tests pass.

josh-bernstein · 2017-10-23T17:52:24Z

@tfili When I do git pull origin master it says 'already up-to-date'.
Same if I checkout master, git pull, then go back to my branch and try to merge.

hpinkos · 2017-10-23T17:54:37Z

@josh-bernstein Make sure you're fetching master from our repo: https://help.github.com/articles/syncing-a-fork/

josh-bernstein · 2017-10-23T18:17:10Z

@hpinkos Thank you for the instructions, but I think I'm missing something.

I've tried different combinations of git fetch upstream, git fetch AnalyticalGraphicsInc, git pull upstream master, git pull AnalyticalGraphicsInc:master`, but they all give errors.

Any advice would be appreciated.

Thank you.

hpinkos · 2017-10-23T18:18:40Z

@josh-bernstein make sure you set upstream first
git remote add upstream [email protected]:AnalyticalGraphicsInc/cesium.git

josh-bernstein · 2017-10-23T18:22:09Z

@hpinkos @tfili Ok, I merged master. Let's see if that works.

josh-bernstein · 2017-10-23T19:24:20Z

Oh no, it failed again!

hpinkos · 2017-10-23T19:29:49Z

Source/DataSources/KmlDataSource.js

@@ -1140,6 +1146,8 @@ define([

        if (!defined(billboard.image)) {
            billboard.image = dataSource._pinBuilder.fromColor(Color.YELLOW, 64);
+        } else if (billboard.image == false) {


@josh-bernstein this is why CI failed on eslint. == should be ===

@hpinkos This condition is only true in the desired cases if I use ==, not ===.
I could remove this 'else if' block altogether and it will still function correctly. I just think it is more intuitive to set the value back from false to undefined

Do } else if (!billboard.image) { instead

Or what is the value of billboard.image here? Is it 'false' instead of false?
In that case, do } else if (billboard.image === 'false') {

You explicitly set billboard.image to false. === should work in this case because the type and value will match.

@hpinkos billboard.image instanceof ConstantProperty throws error saying ConstantProperty is undefined. However, everything seems to work if I do } else if (billboard.image && !billboard.image.getValue()) {

At the top of the file, require in ConstantProperty. See how we require in CompositePositionProperty in this file for an example. We want to make sure it's a constant property because other property types require an argument passed into getValue

@hpinkos In what other situation would billboard.image be defined but have a value of false?

Okay. I had to double check throughout the code but it looks like if it is defined it will always be a ConstantProperty. So we can safely just do } else if (!billboard.image.getValue()) {. I was just being cautious.

@hpinkos Ok sounds good. Thanks for checking it out!

josh-bernstein · 2017-10-23T21:26:00Z

@tfili @hpinkos Looks like it's good now!

hpinkos · 2017-10-23T21:31:34Z

Thanks for fixing this @josh-bernstein!

josh-bernstein · 2017-10-23T21:32:24Z

@hpinkos My pleasure!

Fix bug where yellow icon is shown when icon tag is empty CesiumGS#5819

1fdc621

josh-bernstein closed this Sep 7, 2017

Revert "Fix bug where yellow icon is shown when icon tag is empty Ces…

b59cba1

…iumGS#5819" This reverts commit 1fdc621.

josh-bernstein reopened this Sep 7, 2017

Fix bug with placemarks in imported KML, where placemarks with no spe…

13a6606

…cified icon are displayed with yellow icon CesiumGS#5819

Josh Bernstein added 5 commits September 14, 2017 21:16

Add unit test and supporting KML file CesiumGS#5819

ab50ab5

Revert "Add unit test and supporting KML file CesiumGS#5819"

135d396

This reverts commit ab50ab5.

Add KML file to be used in test CesiumGS#5819

8898faf

Add unit test for icon tag with no image CesiumGS#5819

515de90

Merge pull request #1 from AnalyticalGraphicsInc/master

35d690f

Merge master into my fork

Resolve merge conflict CesiumGS#5819

983ffa6

Merge branch 'master' into 5819_fix_for_empty_icon_tag

b9dfceb

tfili requested changes Sep 28, 2017

View reviewed changes

Josh Bernstein added 4 commits September 29, 2017 13:02

Add my name to contributors file CesiumGS#5819

0bf2659

Merge branch 'master' into 5819_fix_for_empty_icon_tag

f373ebe

Add test KML file with no style CesiumGS#5819

5518ba9

Include company name in CONTRIBUTORS.md CesiumGS#5819

31efd9d

Josh Bernstein added 3 commits October 18, 2017 12:40

Revert "Cover edge case where IconStyle tag is empty CesiumGS#5819"

df09819

This reverts commit 470810d.

Merge branch 'master' into 5819_fix_for_empty_icon_tag

f6154ca

Simplify code and cover all cases correctly CesiumGS#5819

cb773a6

Merge branch 'master' into 5819_fix_for_empty_icon_tag

fa1e73e

tfili reviewed Oct 23, 2017

View reviewed changes

Josh Bernstein added 2 commits October 23, 2017 18:23

Merge remote-tracking branch 'upstream/master'

9758800

Merge branch 'master' into 5819_fix_for_empty_icon_tag

6b6f55c

hpinkos reviewed Oct 23, 2017

View reviewed changes

Avoid using == CesiumGS#5819

4b0b03a

hpinkos merged commit ae9be87 into CesiumGS:master Oct 23, 2017

Fix bug where yellow icon is shown when icon tag is empty #5819 #5821

Fix bug where yellow icon is shown when icon tag is empty #5819 #5821

Conversation

josh-bernstein commented Sep 7, 2017

cesium-concierge commented Sep 7, 2017

josh-bernstein commented Sep 7, 2017

josh-bernstein commented Sep 7, 2017

hpinkos commented Sep 7, 2017

tfili commented Sep 12, 2017

josh-bernstein commented Sep 12, 2017

biomassives commented Sep 12, 2017 via email

tfili commented Sep 12, 2017

josh-bernstein commented Sep 14, 2017

mramato commented Sep 27, 2017

josh-bernstein commented Sep 27, 2017

mramato commented Sep 27, 2017

pjcozzi commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-bernstein commented Oct 17, 2017

tfili commented Oct 17, 2017

josh-bernstein commented Oct 20, 2017

josh-bernstein commented Oct 20, 2017 • edited Loading

josh-bernstein commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfili commented Oct 23, 2017

josh-bernstein commented Oct 23, 2017

hpinkos commented Oct 23, 2017

josh-bernstein commented Oct 23, 2017

josh-bernstein commented Oct 23, 2017

tfili commented Oct 23, 2017

josh-bernstein commented Oct 23, 2017

hpinkos commented Oct 23, 2017

josh-bernstein commented Oct 23, 2017

hpinkos commented Oct 23, 2017

josh-bernstein commented Oct 23, 2017

josh-bernstein commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-bernstein Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

hpinkos Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-bernstein commented Oct 23, 2017

hpinkos commented Oct 23, 2017

josh-bernstein commented Oct 23, 2017

josh-bernstein commented Oct 20, 2017 •

edited

Loading

josh-bernstein Oct 23, 2017 •

edited

Loading

hpinkos Oct 23, 2017 •

edited

Loading