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

Alpha - Changing Tile format Messes up States #11

Closed
spdermn02 opened this issue Aug 7, 2023 · 4 comments · Fixed by #12
Closed

Alpha - Changing Tile format Messes up States #11

spdermn02 opened this issue Aug 7, 2023 · 4 comments · Fixed by #12
Assignees
Labels
bug Something isn't working

Comments

@spdermn02
Copy link
Owner

So I had a tiled progress bar, that i originally setup as 1 column, 2 rows, but i really wanted it left to right and so i changed it to a tile setup of 2 columns and 1 row. As soon as i did that, the states for the original set were removed, but only 1 state for the new one was created.

Will need to validate if this is a code issue or something with requests being made into Touch Portal and it not being handled correctly there.

image

@spdermn02 spdermn02 added the bug Something isn't working label Aug 7, 2023
@mpaperno
Copy link
Collaborator

mpaperno commented Aug 7, 2023

That's a bit strange. The fact that there is now a "col 2" state (and no "row 2") suggests it's working correctly in terms of removing the old ones... and new states are created the same way each time as long as DynamicIcon.stateCreated is false. Which it must have been, otherwise that "col 2" state wouldn't have been created.

Try it again? I have seen TP miss a createState message if it comes soon after a remove... very occasionally. Seems unlikely that would happen here, but maybe since the remove/create messages are so close together.

Looking at the code, only other thing I can think of offhand is that the TP Client threw an exception on one of the createState() calls... but that would be logged I think.

The part where it deletes the previous states when changing tiling parameters is at index.ts:276. Though that part doesn't seem to be the issue.

🤨

@mpaperno
Copy link
Collaborator

mpaperno commented Aug 7, 2023

Seems to be a TP thing... I can recreate the issue but only randomly. The missing state is usually col1,row1 because it always exists so it's deleted and then created rapidly. My guess is some timing in TP deletes it after the creation message comes in.

I added this delay and now can't recreate the issue:

 278:                    clearIconStates(icon);
 279:                    // set the tile property after clearing out any old states.
 280:                    Point.set(icon.tile, tile);
+281:                    await new Promise(resolve => setTimeout(resolve, 50));
 282:                }

Kinda ugly but maybe a workaround.

@spdermn02
Copy link
Owner Author

Yeah I figured as much, it has to be related to the rapid remove and create. Because I also saw if you increase one of the parameters, like row to 3 from 2 it has the same effect. It only ended up showing the row 3 state because the first two were removed. Has to be an issue with speed and TP. I don't love the idea of having to put in those sleep type statements. But ... unsure how else it would work. Only other way is determine only the new states that are needed and don't remove the old ones if there is overlap...

mpaperno added a commit that referenced this issue Aug 8, 2023
* Smarter updates When tiling properties change, does not remove any states which will still exist in the new tiling layout (fixes #11);
* Create states explicitly when icons are initially created instead of needing to check before every delivery;
* Drop the `DynamicIcons.stateCreated` property and instead use a default empty `tile` to indicate state(s) need creation;
mpaperno added a commit that referenced this issue Aug 8, 2023
* Smarter updates When tiling properties change, does not remove any states which will still exist in the new tiling layout (fixes #11);
* Create states explicitly when icons are initially created instead of needing to check before every delivery;
* Drop the `DynamicIcons.stateCreated` property and instead use a default empty `tile` to indicate state(s) need creation;
@mpaperno
Copy link
Collaborator

mpaperno commented Aug 8, 2023

Only other way is determine only the new states that are needed and don't remove the old ones if there is overlap

Yea, that was the way to go.... the old way was simple (lazy?) but needing a delay is unpalatable.

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 a pull request may close this issue.

2 participants