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

[Feature Request]: Include image size units in the image file name #19161

Closed
adhiamboperes opened this issue Nov 16, 2023 · 6 comments
Closed
Assignees
Labels
enhancement Label to indicate an issue is a feature/improvement

Comments

@adhiamboperes
Copy link

adhiamboperes commented Nov 16, 2023

Is your feature request related to a problem? Please describe.

We recently discovered on Android that not all svgs use the same sizing units. For example, mathImg_* images such as those used for fractions, have dimensions in ex, which we needed to write a custom conversion for. Initially, we were not aware of this issue, leading to a severe bug oppia/oppia-android#5112.

Describe the solution (or solutions) you'd like

We would like the image file names to include the units for width and height.

Otherwise it's ambiguous when looking at the file names without context. We're actually supposed to use only the filenames for sizing to have parity with Web.

Describe alternatives you've considered and rejected

We fall back to the document’s sizing when we can't use filename. We have to retrieve the document sizes from the document itself via the androidsvg lib, but this is not always reliable.

We get the units from the parsed svg itself, the document w/h: For Block svgs, the units are either mm, px or not indicated(just a number with no units), and for a few, they lack the length and width attributes(noticed this in the portuguese translations). And oddly enough, for img_* images, not that many use px but mm(but this is a direct 1:1 conversion, so not an issue for android). These are all rendered block. The androidsvg class handles these all these cases, per https://github.com/BigBadaboom/androidsvg/blob/c73810fa2f0b6335409472896f165079ca7526ef/androidsvg/src/main/java/com/caverock/androidsvg/SVG.java#L614.

All of the mathImg_* images, that are all rendered inline(especially in fractions), all have ex units without exception. The androidsvg lib handles this as well, so we really don't need to worry about it when using documentWidth/height.

Additional context

Debugging doc for the linked issue: https://docs.google.com/document/d/1DCQ6C8bNH5IGRwmzWwq4vrMEA54DpFBmghDU795Y5oc/edit

@adhiamboperes adhiamboperes added enhancement Label to indicate an issue is a feature/improvement triage needed labels Nov 16, 2023
@Chandan-Singh10
Copy link

I am willing to work on this issue please assign me this issue

@seanlip
Copy link
Member

seanlip commented Nov 20, 2023

Hi @adhiamboperes -- just a note, currently we only have two types of images, mathImg and everything else. As you mention, it seems like they have different sizing conventions (sorry, we weren't aware of this either).

But, given the above information, do you still need the images renamed? That would be a somewhat tricky change to pull off because it involves modifying lesson content that refers to those images, and I would rather avoid that if possible. Is it fine for the Android code to base its sizing on whether the image starts with mathImg or not, or does the current naming convention not provide sufficient data in some cases?

@adhiamboperes
Copy link
Author

Hi @seanlip, on Android, we already use the oppia-noninteractive-image and oppia-noninteractive-math tags associated with the two types to differentiate and parse them, so it is reasonable to rely on image type to handle the sizing concerns.

Do you have a possible explanation for the images having different sizing units? Just so that we may possibly mitigate against any future regressions caused by a change in sizing conventions.

@adhiamboperes adhiamboperes removed their assignment Nov 21, 2023
@seanlip
Copy link
Member

seanlip commented Nov 21, 2023

Hi @adhiamboperes -- ah, if you can use the component then that may be fine too.

We haven't really paid attention to keeping the sizing units in the past, but the reason they're inconsistent is because they're generated differently. SVG images in image tags are uploaded by creators through our editor portal. Math expression images, on the other hand, are generated by MathJax through their own process. So I think that's why the treatment of those images ended up different -- they should be regarded as two different "image types" despite having the same extension.

Does that help (and, should we close this issue if there isn't anything further to do here)?

Thanks!

@adhiamboperes
Copy link
Author

Thanks @seanlip. We can close this issue and document the discussion android side.

@seanlip
Copy link
Member

seanlip commented Nov 21, 2023

Sounds good. Thanks!

@seanlip seanlip closed this as not planned Won't fix, can't repro, duplicate, stale Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Label to indicate an issue is a feature/improvement
Projects
None yet
Development

No branches or pull requests

3 participants