Replace Volley with Glide in the PostsListAdapter #7989
Conversation
There was a problem hiding this comment.
@malinajirka I dropped some comments/questions. I've also tested the PR and it seems to be working well.
| break; | ||
| default: | ||
| if (BuildConfig.DEBUG) { | ||
| throw new IllegalStateException("Missing switch case."); |
| ReplaceWith("imageManager.load(imageView, resourceId, scaleType)", | ||
| "org.wordpress.android.util.image.ImageManager")) | ||
| @JvmOverloads | ||
| fun loadImage(imageView: ImageView, resourceId: Int, scaleType: ImageView.ScaleType = CENTER) { |
There was a problem hiding this comment.
This method doesn't seem to be used, am I missing something? Adding a deprecated, unused method feels really weird to me. I am not familiar with the history of the project, could you let me know why we needed to add these deprecated methods in the first place?
Actually, I just checked and none of them seems to be used. We have this comment Object for backward compatibility with code which doesn't support DI, but why wouldn't the DI be supported?
I am really confused. I have a few more questions, but I'll wait until I understand a bit more about what's going on.
There was a problem hiding this comment.
There are basically two related reasons for introducing deprecated methods
- It's easier to do incremental refactoring than trying to refactor everything at once.
- suppose you have an activity/fragment, which doesn't use dagger yet. It's easier to migrate from Volley to Glide in first iteration and don't care about dagger and then add support for dagger in the second iteration. This is just a dump example, as adding dagger to an activity/fragment is one line of code, but I hope you get the point.
- Some classes don't use dagger and adding dagger isn't a good idea. Great example is ReaderThumbnailStrip. It's a View class which contains business logic. It needs to be refactored, but doing the refactoring is out of the scope of this PR (even of this project).
Since we want everyone to be able to use the ImageManager without the need for huge refactoring, I believe it should provide static methods. On the other hand we want to make sure no-one uses the static methods when they can use Dagger, hence the methods are deprecated.
Let me know if you have any further questions or if you believe I should take another approach:).
There was a problem hiding this comment.
I think the problem is that these deprecated methods are not currently in use (at least in this branch). Until they are actually needed, I'd strongly advice against adding them. I can't find our documentation for it, but I know that we don't want any unused code in our repositories. If they are actually in use and I am just missing it, please let me know.
Specifically for this case, adding that code right now compared to adding that code when it's needed, it makes it not a part of the correct PR review. I can't comment on that code at all right now, because I don't know how it's going to be used. However, if that deprecated method was added in a PR that uses it and there is a better way to do it without the need of the deprecated method, I can say that let's not add it and do this alternative way instead. Does that make sense?
Side note: For the deprecated methods, creating a new instance ImagePlaceholderManager() for every time it's called is kind of a big waste. If the caller is a listview and this is called many times, we'd consume a lot of memory for no reason.
There was a problem hiding this comment.
I have always been a fan of no dead/unused code rule and I think every company should have such rule. On the other hand every rule has valid exceptions. I believe this is a case where it makes sense to make an exception.
-
The static methods don't do anything else than call the original instance methods. It's kind of a simple adapter in a way. All the instance methods are used so you can post logic related comments there.
-
Implementing these static methods during development of the class has some advantages.
- The ImageManager class is more of 'library' class which provides an interface. Other developers won't need to touch the ImageManager class and they can just use it.
- If we don't have these static methods, other developers (or me in the future) might not realize they should annotate them with
@Deprecatedwhen they create them.
- I basically agree with your point that it's hard to review an unused method and you can't propose an alternative way, because you don't now how it's going to be used. However, this PR introduces just another static methods, so we have all the instance methods available in the static context. I'd completely agree with you, if I introduced first static method and it wasn't used.
Here is an example of a usage.
Probably the biggest advantage of implementing the methods before they are actually used comes when more people are working on the refactoring. It prevents everyone from creating their own version of such method. Since I'm the only one working on this, I'm okay with removing all the unused methods. However, I'd like to hear another opinion on this topic, so when we work on refactoring in a group I know whether it's a valid approach or not.
@hypest Your comments are always wise. Could you please give us your opinion on this? Thanks!
There was a problem hiding this comment.
Side note: For the deprecated methods, creating a new instance ImagePlaceholderManager() for every time it's called is kind of a big waste. If the caller is a listview and this is called many times, we'd consume a lot of memory for no reason.
Tbh I can't think of any other way -> we could let the client create an instance of the ImageManager and send it to the method, but it wouldn't make sense to have static methods in the first place (the client could directly call the instance methods). Disadvantage of calling instance methods on a manually created instance is, that those methods are not Deprecated. I guess I could create a lazy loaded singleton, but it feels like overkill. The ImageManager is basically just an Object with few extra methods. It's creation and memory footprint should be neglectable. Moreover current garbage collection techniques are optimized for collecting many short-lived objects so the memory footprint might be actually lower than creating one longer living object. There are several great articles about this topic (for instance here and here). Nevertheless, I'll be happy to rewrite it. Do you have any specific suggestion?
There was a problem hiding this comment.
Could you please give us your opinion on this? Thanks!
Thanks for pinging @malinajirka !
Here is an example of a usage.
So, it seems there is a usage for one of the static methods. I guess it would help to merge develop in this PR so that becomes easier to follow.
The ImageManager class is more of 'library' class which provides an interface.
The companion object is effectively a utility class and since it's not part of an actual library, the incentive to cater for the needs of a random user of the lib is low I think.
In this case, and since this was brought up during the review, I'd be leaning towards cleaning up the companion object, only leaving in the static method that is actually used.
I'd expect any future static method additions to the companion object to "copy" the one method already defined and mark it as deprecated on inception. That can be part of the PR review of that future addition.
I'd also advocate that even if there are good (potential) uses the static methods, we'd still like to "push" for using the DI'ed singleton instead. With that in mind, offering a complete set of static methods like currently in the PR would enable people to use them, beating the purpose of "pushing" to DI :)
All in all, I think the utility of the static methods is minimal at the moment and we can easily handle adding more in the future if needed so, I'd recommend not offering unused methods in this PR.
Hope I helped!
There was a problem hiding this comment.
Thank you both;), I'm going to clean it up.
There was a problem hiding this comment.
@malinajirka Is there a reason you don't just add a method like sharedInstance() that simply returns a singleton for the deprecated case? You can deprecate that method and you won't need to implement any other static methods since the regular methods could be used on that singleton. That also fixes both the unused methods and unnecessary memory consumption issues.
|
Thank you @oguzkocer ! I've fixed the issue and I've tried to explain my reasoning behind introducing unused deprecated methods. |
There was a problem hiding this comment.
@malinajirka I added a comment for the latest changes. It should be good to merge after that. Thanks!
| fun clear(imageView: ImageView) { | ||
| ImageManager(ImagePlaceholderManager()).cancelRequestAndClearImageView(imageView) | ||
| } | ||
| @Deprecated("Use injected ImageManager") |
There was a problem hiding this comment.
You can add @JvmStatic to avoid having to call it by ImageManager.Companion.getInstance() and use ImageManager.getInstance() instead.
There was a problem hiding this comment.
I've completely forgotten about that annotation. Thanks!
|
I've added the @JvmStatic annotation. Thanks for the tip;)! |
|
@malinajirka I missed merging this on Friday, really sorry about that. 🤦♂️ Will do that in just a bit! |
|
No worries;)! There is no need to rush. |
Fixes #7960
Replaces Volley with Glide on the
My Site -> Site PagesandMy Site -> Blog Posts. Adds support for animated feature images. Otherwise nothing should change from the user perspective.To test: