feat: 🎸 gif supported by SDWebImage #32

ShadowTourist · 2021-08-09T09:18:23Z

Support gif by SDWebImage

ShadowTourist · 2021-08-09T09:30:33Z

before	after

MarcoEidinger

Overall looks really good!

I noticed that cache has a certain impact on memory. Lets add a section to the README to inform the app developer that app developer decides if/when image cache gets deleted. For example, for onDisappear for AssistantView the following is possible

SDImageCache.shared.clearMemory()
SDImageCache.shared.clearDisk()

Also the README file has to be updated in the following areas

MarcoEidinger · 2021-09-01T23:00:38Z

Sources/SAPCAI/UI/Common/SwiftUI/AvatarView.swift

+                .frame(width: 32, height: 32, alignment: .center)
+                .clipped()
+        } else {
+            Image(systemName: "icloud.slash")


is this new?

MarcoEidinger · 2021-09-01T23:03:00Z

Sources/SAPCAI/UI/Common/UIKit/ImageViewWrapper.swift

+    }
+}
+
+struct ImageViewWrapper<Content>: View where Content: View {


Please add a Preview for ImageViewWrapper

MarcoEidinger · 2021-09-01T23:04:56Z

Sources/SAPCAI/UI/Common/UIKit/ImageViewWrapper.swift

+
+    func updateUIView(_ uiView: UIImageViewWrapper, context: UIViewRepresentableContext<ImageViewRepresentable>) {
+        uiView.imageView.contentMode = self.imageManager.contentMode
+        /// retry strategy missed


what does this comment mean?

MarcoEidinger

Thank yo so much @ShadowTourist , that looks really great!!

@justinguo I assume that is also fine from your side?

Here is my last-minute nit-picking feedback. Once incorporated I will merge the PR into a new branch devBreaking.

Just in case you or Justin wonder why a new branch: there are already changes in dev branch which were not yet published. I might want to roll those out as a patch release but the gif support through SDWebImage is a minor/major release worth due to the breaking changes of switching package dependencies.

MarcoEidinger · 2021-09-02T21:50:09Z

Sources/SAPCAI/UI/AssistantView.swift

@@ -1,4 +1,5 @@
 import Combine
+import SDWebImage


I don't think this is needed here. If so please remove

MarcoEidinger · 2021-09-02T21:51:34Z

README.md

@@ -87,7 +87,7 @@ curl -X POST "<BaseUrl>/connect/v1/channels" \

 - [SAPCommon](https://help.sap.com/doc/978e4f6c968c4cc5a30f9d324aa4b1d7/Latest/en-US/Documents/Frameworks/SAPCommon/index.html) for Logging
 - [SAPFoundation](https://help.sap.com/doc/978e4f6c968c4cc5a30f9d324aa4b1d7/Latest/en-US/Documents/Frameworks/SAPFoundation/index.html) for Network Connectivity and Authentication
- [URLImage](https://github.com/dmytro-anokhin/url-image) for asynchronous image loading in SwiftUI
+- [SDWebImage](https://github.com/SDWebImage/SDWebImage) for asynchronous image loading in SwiftUI


Suggested change

- [SDWebImage](https://github.com/SDWebImage/SDWebImage) for asynchronous image loading in SwiftUI

- [SDWebImage](https://github.com/SDWebImage/SDWebImage) for asynchronous image loading and gif animation in SwiftUI

MarcoEidinger · 2021-09-02T21:57:26Z

README.md

@@ -146,6 +146,9 @@ AssistantView()
  .onDisappear {
    viewModel.cancelSubscriptions()
    dataPublisher.resetConversation()
+    //Clear SDImage Cache


Suggested change

//Clear SDImage Cache

// SAPCAI uses `SDWebImage` and its image caching capabilities but it is the app developers responsibility to clear the cache if that is desired

justinguo · 2021-09-02T22:55:24Z

Overall looks good to me. I tested and it works fine on device.

justinguo · 2021-09-02T23:01:46Z

@ShadowTourist Did you add the retry operation in updateUIView() method? Like if display failure view, can user tap the image again to retry the downloading and decoding?

ShadowTourist · 2021-09-03T01:26:40Z

@ShadowTourist Did you add the retry operation in updateUIView() method? Like if display failure view, can user tap the image again to retry the downloading and decoding?

No, currently image view has no capability of interaction, so retry can be added by SDWebImage, there is a option called SDWebImageRetryFailed for image load failed. If we need add this for ImageViewWrapper?

* feat: 🎸 gif supported by SDWebImage. * feat: 🎸 placeholder and failure support alse README updated * docs: ✏️ update README

* feat: 🎸 gif supported by SDWebImage. * feat: 🎸 placeholder and failure support alse README updated * docs: ✏️ update README Co-authored-by: XiaoYu <33440079+ShadowTourist@users.noreply.github.com>

ShadowTourist force-pushed the change-urlimage-to-sdwebimage branch from 6779dac to d3d8c45 Compare August 9, 2021 09:19

MarcoEidinger added the wip work in progress label Aug 9, 2021

MarcoEidinger changed the title ~~refactor: 💡 SDWebImageSwiftUI Research~~ wip: 💡 SDWebImageSwiftUI Research Aug 9, 2021

MarcoEidinger force-pushed the dev branch from e1ba5c3 to 34e4248 Compare August 10, 2021 23:58

ShadowTourist force-pushed the change-urlimage-to-sdwebimage branch 5 times, most recently from f851191 to e0ceae7 Compare August 27, 2021 10:14

feat: 🎸 gif supported by SDWebImage.

84ce935

ShadowTourist force-pushed the change-urlimage-to-sdwebimage branch from e0ceae7 to 84ce935 Compare September 1, 2021 02:50

ShadowTourist changed the title ~~wip: 💡 SDWebImageSwiftUI Research~~ feat: 🎸 gif supported by SDWebImage Sep 1, 2021

MarcoEidinger requested review from justinguo and MarcoEidinger September 1, 2021 22:28

MarcoEidinger changed the base branch from dev to devBreaking September 1, 2021 22:32

MarcoEidinger suggested changes Sep 1, 2021

View reviewed changes

ShadowTourist force-pushed the change-urlimage-to-sdwebimage branch from 2275708 to c989908 Compare September 2, 2021 03:03

feat: 🎸 placeholder and failure support alse README updated

9becf1a

ShadowTourist force-pushed the change-urlimage-to-sdwebimage branch from c989908 to 9becf1a Compare September 2, 2021 03:19

ShadowTourist requested a review from MarcoEidinger September 2, 2021 05:05

MarcoEidinger suggested changes Sep 2, 2021

View reviewed changes

docs: ✏️ update README

833d6e9

ShadowTourist requested a review from MarcoEidinger September 3, 2021 01:40

MarcoEidinger approved these changes Sep 3, 2021

View reviewed changes

MarcoEidinger merged commit ba6cf23 into SAP:devBreaking Sep 3, 2021

MarcoEidinger pushed a commit that referenced this pull request Sep 30, 2021

feat: 🎸 gif supported through SDWebImage (#32)

66af1a3

* feat: 🎸 gif supported by SDWebImage. * feat: 🎸 placeholder and failure support alse README updated * docs: ✏️ update README

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🎸 gif supported by SDWebImage #32

feat: 🎸 gif supported by SDWebImage #32

ShadowTourist commented Aug 9, 2021 •

edited

Loading

ShadowTourist commented Aug 9, 2021

MarcoEidinger left a comment

MarcoEidinger Sep 1, 2021

MarcoEidinger Sep 1, 2021

MarcoEidinger Sep 1, 2021

MarcoEidinger left a comment

MarcoEidinger Sep 2, 2021

MarcoEidinger Sep 2, 2021

MarcoEidinger Sep 2, 2021

justinguo commented Sep 2, 2021

justinguo commented Sep 2, 2021

ShadowTourist commented Sep 3, 2021

	- [SDWebImage](https://github.com/SDWebImage/SDWebImage) for asynchronous image loading in SwiftUI
	- [SDWebImage](https://github.com/SDWebImage/SDWebImage) for asynchronous image loading and gif animation in SwiftUI

	//Clear SDImage Cache
	// SAPCAI uses `SDWebImage` and its image caching capabilities but it is the app developers responsibility to clear the cache if that is desired

feat: 🎸 gif supported by SDWebImage #32

feat: 🎸 gif supported by SDWebImage #32

Conversation

ShadowTourist commented Aug 9, 2021 • edited Loading

ShadowTourist commented Aug 9, 2021

MarcoEidinger left a comment

Choose a reason for hiding this comment

MarcoEidinger Sep 1, 2021

Choose a reason for hiding this comment

MarcoEidinger Sep 1, 2021

Choose a reason for hiding this comment

MarcoEidinger Sep 1, 2021

Choose a reason for hiding this comment

MarcoEidinger left a comment

Choose a reason for hiding this comment

MarcoEidinger Sep 2, 2021

Choose a reason for hiding this comment

MarcoEidinger Sep 2, 2021

Choose a reason for hiding this comment

MarcoEidinger Sep 2, 2021

Choose a reason for hiding this comment

justinguo commented Sep 2, 2021

justinguo commented Sep 2, 2021

ShadowTourist commented Sep 3, 2021

ShadowTourist commented Aug 9, 2021 •

edited

Loading