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

fix: problem when deleting capture areas #198

Merged
merged 15 commits into from
Jun 30, 2023
Merged

Conversation

BeierKevin
Copy link
Contributor

@BeierKevin BeierKevin commented Jun 2, 2023

PR description

There was a bug with the capture area array not really updating the rendered list properly because its not reactive etc. The solution may not be really good tho.

Definition Of Done (DoD)

This PR can be squashed / merged if

  • a developer is assigned
  • the PR is NOT estimated
  • the PR is labeled
  • the PR is NOT assigned to the current sprint
  • a meaningful title has been set according to https://www.conventionalcommits.org/
  • the PR is described in detail
  • the PR links to an issue
  • the PR has been reviewed

Add additional conditions here if necessary for this PR

fix: #186

@BeierKevin BeierKevin requested a review from Claiyc June 2, 2023 09:30
@BeierKevin BeierKevin self-assigned this Jun 2, 2023
@BeierKevin BeierKevin requested review from roman533 and 2000eBe June 2, 2023 17:16
roman533
roman533 previously approved these changes Jun 3, 2023
Copy link
Contributor

@roman533 roman533 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple solution, works

Copy link
Member

@Claiyc Claiyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a bigger issue than it seems… We cannot just reassign ca ids, as the VIGAD-API redis db structure relies on them (and therefore potential third party apps as well). A solution may be to have unique ids for capture areas (randomly generated). However for this solution we should provide the ability for a user to manually override the id of a capture area to keep consistency between sessions.

@BeierKevin
Copy link
Contributor Author

BeierKevin commented Jun 4, 2023

If there should be the option that the user can change the ID of a capture area i need to change a lot ofthings for example should the id then be only a number and what rules should this id follow if any ?

@Claiyc
Copy link
Member

Claiyc commented Jun 4, 2023

If there should be the option that the user can change the ID of a capture area i need to change a lot ofthings for example should the id then be only a number and what rules should this id follow if any ?

I guess it will be easier if it is alphanumeric (so we definitely won't run into issues with collisions). However it needs to be URI conform as well. Other than that, no rules that I can think of. Maybe not too long (idk, 6 characters should be enough?)

@BeierKevin
Copy link
Contributor Author

If this only alphanumeric isn't it always URI conform ?

src/composables/useTokenGenerator/useTokenGenerator.ts Outdated Show resolved Hide resolved
src/proc/Vigad.ts Show resolved Hide resolved
src/proc/Vigad.ts Show resolved Hide resolved
@BeierKevin BeierKevin requested a review from Claiyc June 11, 2023 21:40
@BeierKevin
Copy link
Contributor Author

This is just a workaround, because i cant fix it really

Copy link
Member

@Claiyc Claiyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to get the job done

@Claiyc Claiyc merged commit a152db1 into main Jun 30, 2023
@Claiyc Claiyc deleted the fix/186-deleting-capture-area branch June 30, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug when having multiple Capture Areas, and deleting one out of order
3 participants