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

[MPQEditor] Handle invalid VISQ data #1561

Merged
merged 1 commit into from
May 24, 2023

Conversation

stamalakhov
Copy link
Contributor

@stamalakhov stamalakhov commented May 22, 2023

This commit implements handling of invalid VISQ file.

It's correctness is approved below:

simplescreenrecorder-2023-05-23_11.37.45.mp4

Fresh draft: #1543
Full draft: #1505
Related: #1491

ONE-vscode-DCO-1.0-Signed-off-by: s.malakhov [email protected]

@stamalakhov stamalakhov requested review from jyoungyun and dayo09 May 22, 2023 06:59
@stamalakhov stamalakhov self-assigned this May 22, 2023
dayo09
dayo09 previously approved these changes May 23, 2023
Copy link
Contributor

@dayo09 dayo09 left a comment

Choose a reason for hiding this comment

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

LGTM Thanks.
However, additionally, I think it would be better if
(1) non-visq files are unabled to be selected from the window initially.
(2) the circle viewer doesn't flick.

And I wonder what would happen if any non-visq file with *.visq.json postfix is selected.

@stamalakhov
Copy link
Contributor Author

stamalakhov commented May 23, 2023

@dayo09 Thank you for your review.

non-visq files are unabled to be selected from the window initially.

By default selector is set to target files, which are *.visq.json.

And I wonder what would happen if any non-visq file with *.visq.json postfix is selected.

It will make sure that file doesn't contain error section or meta section, and will reboot in selector mode.
Please see

if (!("error" in this._visqData) || !("meta" in this._visqData)) {

@stamalakhov
Copy link
Contributor Author

stamalakhov commented May 23, 2023

@dayo09

the circle viewer doesn't flick.

PR can be reworked. However it will be a large change.

@dayo09
Copy link
Contributor

dayo09 commented May 23, 2023

@stamalakhov I don't mind whether you proceed it in next PR or here. :-D

By default selector is set to target files, which are *.visq.json.

It will make sure that file doesn't contain error section or meta section, and will reboot in selector mode.
Please see

Oh sure. Those concerns are resolved then.

This commit implements handling of invalid VISQ file.

ONE-vscode-DCO-1.0-Signed-off-by: s.malakhov <[email protected]>
@stamalakhov stamalakhov force-pushed the manual_MPQ_failed_to_load_br branch from c75df57 to 7c0db20 Compare May 23, 2023 08:43
@stamalakhov
Copy link
Contributor Author

stamalakhov commented May 23, 2023

@dayo09
I've reworked PR and also changed description (please see attached .mp4) . No flickering now.

@stamalakhov stamalakhov requested a review from dayo09 May 23, 2023 08:48
Copy link
Collaborator

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

@seanshpark seanshpark merged commit b3ecd48 into Samsung:main May 24, 2023
@stamalakhov stamalakhov deleted the manual_MPQ_failed_to_load_br branch May 24, 2023 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants