Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order of persistent marker is ignored #858

Open
fax-sma opened this issue Aug 21, 2024 · 4 comments
Open

Order of persistent marker is ignored #858

fax-sma opened this issue Aug 21, 2024 · 4 comments

Comments

@fax-sma
Copy link

fax-sma commented Aug 21, 2024

How to reproduce

Add two persistent marker to one chart that overlap.

Observed behavior

The order of the markers is not preserved from the original map.

Expected behavior

I would like to control the order of the markers by the order of the map I put into the chart.

Vico version(s)

1.15.0

Android version(s)

All

Additional information

I have looked down into BasicChart and found that the holder for persistent markers is a HashMap. By changing it to a LinkedHashMap the expected behavior could be achieved.

@fax-sma fax-sma added the bug label Aug 21, 2024
@Gowsky
Copy link
Member

Gowsky commented Sep 14, 2024

This has been addressed in Vico 2 — the order of persistent markers is now preserved. However, this is an enhancement, not a bug fix, since order preservation was never suggested. Vico 1 receives only backward-compatible bug fixes, so this change won’t be ported to it. Please consider updating to Vico 2.

@Gowsky Gowsky closed this as not planned Won't fix, can't repro, duplicate, stale Sep 14, 2024
@Gowsky Gowsky removed the bug label Sep 14, 2024
@fax-sma
Copy link
Author

fax-sma commented Sep 16, 2024

However, this is an enhancement, not a bug fix

In my opinion this is a bug because it leads to unwanted behavior. You can't predict which marker will be rendered first. So it can clearly break a fully functional UI.

Please consider updating to Vico 2.

We may consider this in the future but since there are breaking changes nearly every alpha-release we won't do that as of now.

@Gowsky
Copy link
Member

Gowsky commented Sep 17, 2024

@fax-sma, a bug causes incorrect behavior. There should be a reasonably objective standard by which it's an error. The lack of order preservation alone isn't categorically incorrect, even if creates an undesirable limitation. A consumer can still depend on behavior that is unwanted in certain cases, and a change in this behavior would be a breaking change for such a consumer.

However, it turns out that persistent markers are ordered unpredictably in Vico 1. They may be ordered by x value, but they aren't always. This unpredictability does constitute a bug. It also means that a consumer cannot reasonably depend on the current behavior.

To fix the bug, we have to introduce some kind of ordering, and keeping the order in which the consumer added the markers is the best choice. Therefore, we will be making this change to Vico 1, and I'm reopening the issue.

@Gowsky
Copy link
Member

Gowsky commented Oct 13, 2024

Hello, Vico 1.16.0-alpha.1 addresses this. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants