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

Jump to bottom

Improve Screenshot validation #450

Merged
merged 3 commits into from Dec 22, 2022
Merged

Improve Screenshot validation #450

merged 3 commits into from Dec 22, 2022

Conversation

JakobDev
Copy link
Contributor

I added the following checks:

  • At least one Screenshot must be a default
  • Width and Height must be Integers
  • source and thumbnail are the only allowed values for a Image type
  • If a Image type is thumbnail, it must have 'width' and 'height'
  • Only one Image with type source per language is allowed for each Screenshot
  • If a Screenshot is a Image, it must contain a source Image

ximion
ximion requested changes Dec 15, 2022
View changes
Copy link
Owner

@ximion ximion left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! In general those are useful improvements, I just had a few comments on the code itself that need to be addressed before merging this (a rebase on master might also be useful, a lot of previous warnings are now strict errors).

src/as-validator-issue-tag.h Outdated Show resolved Hide resolved
src/as-validator-issue-tag.h Outdated Show resolved Hide resolved
src/as-validator-issue-tag.h Outdated

{ "screenshot-image-missing-width",
AS_ISSUE_SEVERITY_ERROR,
N_("The width property must be present if the type is thumbnail")
Copy link
Owner

Choose a reason for hiding this comment

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

...same for things like width if used as meaning the exact, literal name of a tag or property (put in backticks, add translator hint).

src/as-validator-issue-tag.h Outdated Show resolved Hide resolved
src/as-validator-issue-tag.h Outdated Show resolved Hide resolved
src/as-validator.c Outdated

if (iter->type != XML_ELEMENT_NODE)
continue;

scr_kind_str = as_xml_get_prop_value (iter, "type");
if (g_strcmp0 (scr_kind_str, "default") == 0)
if (g_strcmp0 (scr_kind_str, "default") == 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't do this. Use as_screenshot_kind_from_string and check whether as_screenshot_kind_from_string (scr_kind_str) == AS_SCREENSHOT_KIND_DEFAULT (more consistent, easier for refactoring, if you get an UNKNOWN you can already raise a validation warning.

src/as-validator-issue-tag.h Outdated Show resolved Hide resolved
src/as-validator.c Outdated
@@ -1257,10 +1264,69 @@ as_validator_check_screenshots (AsValidator *validator, xmlNode *node, AsCompone
if (iter2->type != XML_ELEMENT_NODE)
continue;

g_autofree gchar *screenshot_width = NULL;
g_autofree gchar *screenshot_height = NULL;
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't add declarations after statements for now...

src/as-validator.c

image_type = as_xml_get_prop_value (iter2, "type");
if (image_type == NULL)
image_type = "source";
Copy link
Owner

Choose a reason for hiding this comment

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

Please use as_screenshot_kind_from_string, that will do the right thing here and you get an enum value to compare in the subsequent lines, instead of comparing strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to return only if it is default. I did not find a function to check if a type is source or thumbnail

Copy link
Owner

Choose a reason for hiding this comment

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

You will need to use as_image_kind_from_string for that

@ximion
Copy link
Owner

ximion commented Dec 22, 2022

There will be a release today/tomorrow, would be nice to get this into the last release of this year :-)

ximion
ximion requested changes Dec 22, 2022
View changes
src/as-validator.c

image_type = as_xml_get_prop_value (iter2, "type");
if (image_type == NULL)
image_type = "source";
Copy link
Owner

Choose a reason for hiding this comment

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

You will need to use as_image_kind_from_string for that

@ximion
Copy link
Owner

ximion commented Dec 22, 2022

Can you also rebase this patch on master? Currently it can not be merged easily due to conflicts.

@ximion ximion merged commit 4a16c6d into ximion:master Dec 22, 2022
8 checks passed
ximion added a commit that referenced this pull request Dec 22, 2022
@ximion
trivial: Fix style issues and memory leak
e1c5241 
This commit edits 4a16c6d to make it
follow the style of the rest of the code, and also fixes a memory leak.

CC: #450
@ximion
Copy link
Owner

ximion commented Dec 22, 2022

I merged this and fixed all mentioned issues in a follow-up commit. Thank you for the patch!

@JakobDev JakobDev deleted the screenshotvalidate branch January 10, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ximion ximion ximion requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@JakobDev @ximion