Fix #1020: Rendering of Svg images from server. #1019

veena14cs · 2020-04-24T10:28:49Z

Explanation

This PR fixes issue #1020. It implements rendering of svg images from server.

For Testing

In GlideImageLoader on line number 49 replace model with url "https://cdn.shopify.com/s/files/1/0496/1029/files/Freesample.svg"
In assets folder goto fraction_exploration0.json file. Search for image name img_20180121_113315_pqwqhf863w_height_565_width_343 and replace png to svg.

rt4914

Add test cases to add various public functions in this PR.

Otherwise as per your suggested changes your PR is working correctly. Thanks.

utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt

utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt

utility/src/main/java/org/oppia/util/parser/SvgDecoder.kt

utility/src/main/java/org/oppia/util/parser/SvgDrawableTranscoder.kt

utility/src/main/java/org/oppia/util/parser/SvgEncoder.kt

utility/src/main/java/org/oppia/util/parser/SvgSoftwareLayerSetter.kt

veena14cs · 2020-04-24T11:02:06Z

Add test cases to add various public functions in this PR.

There is issue #277 that need to have testcase for image loading. So I haven't added it here. Issue is mentioned in UrlImageParser file.

aggarwalpulkit596 · 2020-04-27T06:17:14Z

Are we handling the fallback case if the image is not an SVG?

veena14cs · 2020-04-27T06:19:06Z

Are we handling the fallback case if the image is not an SVG?

Yes.

aggarwalpulkit596 · 2020-04-27T06:24:28Z

We can add one more case though to render the url as png if svg fails because some websites have a redirect mechanism in svg url which redirects to png url

aggarwalpulkit596 · 2020-04-27T06:29:11Z

@veena14cs there is an error function in glide which executes if the image fails to render we can add png/svg clause in case of a failure

veena14cs · 2020-04-27T06:37:27Z

@veena14cs there is an error function in glide which executes if the image fails to render we can add png/svg clause in case of a failure

In that case you can create a PR and will check it.

rt4914

Non-flaky tests are failing, else looks good.

utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt

nikitamarysolomanpvt

non_flaky_tests are failing PTAL

BenHenning · 2020-05-05T22:51:55Z

Thanks @veena14cs--in the future please assign me the pull request once you need some sort of action item from me (e.g. answering a question or doing another review) so that I can find it more easily. Thanks! :)

I will try to take a look today.

veena14cs · 2020-05-06T06:41:02Z

Thanks @veena14cs--in the future please assign me the pull request once you need some sort of action item from me (e.g. answering a question or doing another review) so that I can find it more easily. Thanks! :)

I will try to take a look today.

Sure @BenHenning thanks.

BenHenning

Thanks @veena14cs! Just a few more comments.

Also, can you make sure all comment threads that are resolved are marked as resolved, otherwise there's a clear question to follow up on them? That makes it easier for me to go back and comment on old threads.

utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt

utility/src/main/java/org/oppia/util/parser/SvgEncoder.kt

utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt

utility/src/main/java/org/oppia/util/parser/SvgSoftwareLayerSetter.kt

utility/src/main/java/org/oppia/util/parser/SvgDecoder.kt

utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt

veena14cs · 2020-05-07T07:59:39Z

Thanks @veena14cs! Just a few more comments.

Also, can you make sure all comment threads that are resolved are marked as resolved, otherwise there's a clear question to follow up on them? That makes it easier for me to go back and comment on old threads.

@BenHenning I have addressed your comments also I changed some of the implementation and removed some files which may not be necessary and could load svg's without any issues.

BenHenning

Thanks @veena14cs! I reopened a couple of the past conversation threads--please resolve those & my new comment before submitting. Otherwise the changes look good. Thanks for implementing this!

If you have any questions about my comments, please ask or reassign the PR to me before submitting.

utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt

veena14cs · 2020-05-11T06:50:42Z

Thanks @veena14cs! I reopened a couple of the past conversation threads--please resolve those & my new comment before submitting. Otherwise the changes look good. Thanks for implementing this!

If you have any questions about my comments, please ask or reassign the PR to me before submitting.

Changed the implementation as per the discussion. @BenHenning Please take a final look before I submit this PR.

BenHenning · 2020-05-12T02:02:00Z

utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt

@@ -93,7 +94,7 @@ class UrlImageParser private constructor(
    }
  }

-  class UrlDrawable : BitmapDrawable() {
+  class UrlDrawable : BitmapDrawable(){


I suggest reverting this (e.g. BitmapDrawable() {).

BenHenning

Thanks @veena14cs--this LGTM!

Added Svgencoder

22af9e9

veena14cs mentioned this pull request Apr 24, 2020

Fix: Rendering of Svg images from server[DO NOT MERGE]. #1015

Closed

veena14cs requested review from rt4914 and nikitamarysolomanpvt April 24, 2020 10:32

veena14cs assigned rt4914 and nikitamarysolomanpvt Apr 24, 2020

veena14cs changed the title ~~Fix: Rendering of Svg images from server[DO NOT MERGE].~~ Fix #1020: Rendering of Svg images from server. Apr 24, 2020

Update UrlImageParser.kt

873b301

rt4914 suggested changes Apr 24, 2020

View reviewed changes

rt4914 assigned veena14cs and unassigned rt4914 Apr 24, 2020

veena14cs assigned BenHenning Apr 24, 2020

veena14cs requested a review from BenHenning April 24, 2020 11:08

veena14cs added 2 commits April 24, 2020 16:52

updated kdoc

59d534c

Update SvgSoftwareLayerSetter.kt

425bdeb

Update SvgEncoder.kt

d6e2aeb

veena14cs assigned rt4914 and unassigned veena14cs Apr 27, 2020

veena14cs requested a review from rt4914 April 27, 2020 08:22

rt4914 suggested changes Apr 28, 2020

View reviewed changes

utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt Outdated Show resolved Hide resolved

rt4914 assigned veena14cs and unassigned rt4914 Apr 28, 2020

nikitamarysolomanpvt suggested changes Apr 28, 2020

View reviewed changes

nikitamarysolomanpvt removed their assignment Apr 28, 2020

BenHenning assigned BenHenning and unassigned veena14cs May 5, 2020

Fixed lint errors

5180b82

BenHenning requested changes May 6, 2020

View reviewed changes

BenHenning assigned veena14cs and unassigned BenHenning May 6, 2020

veena14cs added 2 commits May 7, 2020 12:49

updated new changes

cd1e736

removed encoder file.

bb98f58

veena14cs assigned BenHenning and unassigned veena14cs May 7, 2020

veena14cs added 2 commits May 7, 2020 13:30

nit changes

2da9c43

Update GlideImageLoader.kt

16de4b4

BenHenning approved these changes May 8, 2020

View reviewed changes

utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt Show resolved Hide resolved

BenHenning assigned veena14cs and unassigned BenHenning May 8, 2020

updated from PictureDrawable to Picture.

f1df588

veena14cs assigned BenHenning and unassigned veena14cs May 11, 2020

BenHenning reviewed May 12, 2020

View reviewed changes

BenHenning approved these changes May 12, 2020

View reviewed changes

BenHenning assigned veena14cs and unassigned BenHenning May 12, 2020

veena14cs added 2 commits May 12, 2020 11:07

Update UrlImageParser.kt

8737837

Update GlideImageLoader.kt

ec6f418

veena14cs merged commit d16d6d6 into develop May 12, 2020

veena14cs deleted the render-svg branch May 12, 2020 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1020: Rendering of Svg images from server. #1019

Fix #1020: Rendering of Svg images from server. #1019

veena14cs commented Apr 24, 2020 •

edited

rt4914 left a comment

veena14cs commented Apr 24, 2020

aggarwalpulkit596 commented Apr 27, 2020

veena14cs commented Apr 27, 2020

aggarwalpulkit596 commented Apr 27, 2020

aggarwalpulkit596 commented Apr 27, 2020

veena14cs commented Apr 27, 2020

rt4914 left a comment

nikitamarysolomanpvt left a comment

BenHenning commented May 5, 2020 •

edited

veena14cs commented May 6, 2020

BenHenning left a comment

veena14cs commented May 7, 2020

BenHenning left a comment

veena14cs commented May 11, 2020

BenHenning May 12, 2020

BenHenning left a comment

Fix #1020: Rendering of Svg images from server. #1019

Fix #1020: Rendering of Svg images from server. #1019

Conversation

veena14cs commented Apr 24, 2020 • edited

Explanation

rt4914 left a comment

Choose a reason for hiding this comment

veena14cs commented Apr 24, 2020

aggarwalpulkit596 commented Apr 27, 2020

veena14cs commented Apr 27, 2020

aggarwalpulkit596 commented Apr 27, 2020

aggarwalpulkit596 commented Apr 27, 2020

veena14cs commented Apr 27, 2020

rt4914 left a comment

Choose a reason for hiding this comment

nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

BenHenning commented May 5, 2020 • edited

veena14cs commented May 6, 2020

BenHenning left a comment

Choose a reason for hiding this comment

veena14cs commented May 7, 2020

BenHenning left a comment

Choose a reason for hiding this comment

veena14cs commented May 11, 2020

BenHenning May 12, 2020

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

veena14cs commented Apr 24, 2020 •

edited

BenHenning commented May 5, 2020 •

edited