Skip to content

Fix drag annotation blink issue#639

Merged
Chaoba merged 2 commits intomainfrom
kl-blink-annotation
Sep 15, 2021
Merged

Fix drag annotation blink issue#639
Chaoba merged 2 commits intomainfrom
kl-blink-annotation

Conversation

@Chaoba
Copy link
Copy Markdown
Contributor

@Chaoba Chaoba commented Sep 14, 2021

PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Add example if relevant.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Fix drag annotation blink issue</changelog>.

Summary of changes

In this pr, we will remove the drag annotation from the drag source only when the map is idle and 1 sec later, so there will be no blink issue after drag.

Before:

device-2021-08-20-142132.mp4

After:

device-2021-09-14-133809.mp4

User impact (optional)

@Chaoba Chaoba added the bug 🪲 Something isn't working label Sep 14, 2021
@Chaoba Chaoba requested a review from a team September 14, 2021 05:40
@Chaoba Chaoba self-assigned this Sep 14, 2021
@kiryldz
Copy link
Copy Markdown
Contributor

kiryldz commented Sep 14, 2021

@Chaoba Fix drag annotation blink; Apply properties to drag layer is pretty confusing imho.
Please better describe how it was fixed stating that we introduce delay when drag is finished before removing drag source.
Also could you please clarify if all those tests updates are related to drag blink issue? if not - can we move it to a separate PR?

@kiryldz
Copy link
Copy Markdown
Contributor

kiryldz commented Sep 14, 2021

Also when I look at the second video when you start dragging annotation again (sec 4-5) it seems that drag start is not behaving as expected, annotation jumps to some position instead of smooth dragging.

@Chaoba Chaoba force-pushed the kl-blink-annotation branch from 5250e1c to 8fc4aef Compare September 14, 2021 06:48
@Chaoba Chaoba changed the title Fix drag annotation blink; Apply properties to drag layer Fix drag annotation blink issue Sep 14, 2021
@Chaoba
Copy link
Copy Markdown
Contributor Author

Chaoba commented Sep 14, 2021

Also when I look at the second video when you start dragging annotation again (sec 4-5) it seems that drag start is not behaving as expected, annotation jumps to some position instead of smooth dragging.

We still need to remove the dragging annotation from original source, which will trigger the update for all annotations. We can't skip this step in the current architecture. What we currently do is making it better but not the best.

@kiryldz
Copy link
Copy Markdown
Contributor

kiryldz commented Sep 14, 2021

Also when I look at the second video when you start dragging annotation again (sec 4-5) it seems that drag start is not behaving as expected, annotation jumps to some position instead of smooth dragging.

We still need to remove the dragging annotation from original source, which will trigger the update for all annotations. We can't skip this step in the current architecture. What we currently do is making it better but not the best.

Why is there no such problem when the very first dragging happens?

@pengdev
Copy link
Copy Markdown
Member

pengdev commented Sep 14, 2021

Also when I look at the second video when you start dragging annotation again (sec 4-5) it seems that drag start is not behaving as expected, annotation jumps to some position instead of smooth dragging.

We still need to remove the dragging annotation from original source, which will trigger the update for all annotations. We can't skip this step in the current architecture. What we currently do is making it better but not the best.

Why is there no such problem when the very first dragging happens?

I took a closer look at the first video, I see the annotation being dragged also jumped at the beginning, instead of smooth transition, so in this regard I don't think the behaviour is a regression compared to main branch.

@Chaoba
Copy link
Copy Markdown
Contributor Author

Chaoba commented Sep 14, 2021

I took a closer look at the first video, I see the annotation being dragged also jumped at the beginning, instead of smooth transition, so in this regard I don't think the behaviour is a regression compared to main branch.

Exactly. We have to update all annotations in order to remove the dragging annotation from the original source at the beginning of the drag gesture. Currently we don't have a solution for it.

@pengdev
Copy link
Copy Markdown
Member

pengdev commented Sep 14, 2021

@Chaoba one thought, can we keep both layers during the transition? for example, start merging the dragged annotation to the annotation source, and listen to the MapIdle event, when the MapIdle event is triggered, then remove the dragged layer, wdyt?

@Chaoba
Copy link
Copy Markdown
Contributor Author

Chaoba commented Sep 15, 2021

@Chaoba one thought, can we keep both layers during the transition? for example, start merging the dragged annotation to the annotation source, and listen to the MapIdle event, when the MapIdle event is triggered, then remove the dragged layer, wdyt?

That's what we are doing now. But we always keep two layers.

@Chaoba Chaoba merged commit c0d2a77 into main Sep 15, 2021
@Chaoba Chaoba deleted the kl-blink-annotation branch September 15, 2021 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🪲 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants