Should #placeholder(int) and #error(int) support theme attribute?

[eneim]

I mean in case we want some theme-depended drawable (vector, for example, which could be passed theme's color as attribute, not constant color value). By that a placeholder's drawable could be "lighter" in dark theme, and "darker" in light theme, dynamically. (IMO, caching a placeholder drawable is not necessary, as it doesn't consume too much resource).

The problem now is: Picasso will use a final ApplicationContext for its #getDrawable() action, which will not listen to theme's attributes.

Don't know if you are interested in theming and support libraries or not, but it would be nice if Picasso supports something like ContextCompat.getDrawable().

[NightlyNexus]

I hate to +1, but +1.
Using a given Context to load resources is something Picasso should support.

[JakeWharton]

This is going to be hard to do elegantly. Ideally callers to Picasso.with wouldn't need to re-pass a Context for this behavior but I don't see any way to do that (without something relatively expensive and unsafe).

Otherwise it's Picasso.with(context).load(url).inContext(context)... which is gross. We can change the return type of with either to return something like a RequestCreatorFactory because that's binary incompatible.

[eneim]

How about a context-depended member (say PicassoCompat) who holds a Context instance. I created a snippet to try with Picasso (as follow), but facing a maven dependency problem (so sorry I'm not familiar with it)... Picasso now has an extra PicassoCompat member. For every call to Picasso#with(Context), I retrieve that PicassoCompat instance, to use in getting theme-depended objects only.

final class PicassoCompat {

  private static final WeakHashMap<Context, PicassoCompat> sInstances =
      new WeakHashMap<Context, PicassoCompat>();

  private final Context context;

  private PicassoCompat(Context context) {
    this.context = context;
  }

  static PicassoCompat getInstance(Context context) {
    PicassoCompat instance = sInstances.get(context);
    if (instance == null) {
      instance = new PicassoCompat(context);
      sInstances.put(context, instance);
    }
    return instance;
  }

  Drawable getDrawable(@DrawableRes int resId) {
    return ContextCompat.getDrawable(context, resId);
  }
}

Any picasso.context.getResource().getDrawable(...) would become picasso.picassoCompat.getDrawable(...).

[Hofferic]

Correct me if I'm wrong, but I believe WeakHashMap has strong references to
its keys, only the values are weakly referenced. Feeding something like
that (especially as a static member, which is kept as long as the
Application object exists) with anything but the AppContext (which is
futile because that does not care about themes) would be a recipe for big
memory leaks.

Maybe adding a withTheme() to the RequestBuilder and burdening the caller
with getting that theme would work better, then the builder could resolve
the resource and everything else could remain unchanged. Or giving the
singleton instance a setTheme() which sets a weakreference to be used until
it is overridden or expires...

On Wed, 27 Jan 2016, 07:16 Nam Nguyen Hoai notifications@github.com wrote:

How about a context-depended member (say PicassoCompat) whose holds a
Context instance. I created a snippet to try with Picasso (as follow), but
facing a maven dependency problem... Picasso now has an extra
PicassoCompat member. For every call to Picasso#with(Context), I retrieve
that PicassoCompat instance, to use in getting theme-depended objects only.

final class PicassoCompat {

private static final WeakHashMap<Context, PicassoCompat> sInstances =
new WeakHashMap<Context, PicassoCompat>();

private final Context context;

private PicassoCompat(Context context) {
this.context = context;
}

public static PicassoCompat getInstance(Context context) {
PicassoCompat instance = sInstances.get(context);
if (instance == null) {
instance = new PicassoCompat(context);
sInstances.put(context, instance);
}
return instance;
}

Drawable getDrawable(@DrawableRes int resId) {
return ContextCompat.getDrawable(context, resId);
}
}

Any picasso.context.getResource().getDrawable(...) would become
picasso.picassoCompat.getDrawable(...).

—
Reply to this email directly or view it on GitHub
#1275 (comment).

[eneim]

I just realize that and trying another approach ...

[NightlyNexus]

Since resource handling is the only loading that cares about the theme, I would think an ideal API would be something like picasso.load(resourceId, context). This would mean that the ResourceRequestHandler would need to have the loading-specific Context (not the "global" Context of a Picasso) at load time. If a Request knew about a Context, this might work. I'm talking without checking, but I'll look at this tonight.

[JakeWharton]

In that case it would be an overload for all load methods, as placeholder and error images can also be resources.

[eneim]

After some thought, I notice that almost the UI stuff is done in RequestCreator#into(), so we somehow can start from there IMO. I think there could be room to improve current Target interface.

[NightlyNexus]

By the way, Maven will need to be dropped before any changes here can happen, anyway.
This will be very tricky, considering that Results only care about Bitmaps, never Drawables. This would probably be a case just for resources loaded into Views.

[eneim]

Bitmap is generally downloaded result. When you pass in your custom placeholder resource and error image resource, it would be accessed as Drawable (BitmapDrawable is the one to wrap a Bitmap).

What does your statement Maven will need to be dropped before any changes here can happen mean ? Do you mean they need to change the build system from Maven?

[JakeWharton]

Not strictly true, but for convenient access to the newer APIs, yes.

On Thu, Jan 28, 2016, 8:51 PM Nam Nguyen Hoai notifications@github.com
wrote:

Bitmap is generally downloaded result. When you pass in your custom
placeholder resource and error image resource, it would be accessed as
Drawable (BitmapDrawable is the one to wrap a Bitmap).

What does your statement Maven will need to be dropped before any changes
here can happen ? Do you mean they need to change the build system from
Maven?

—
Reply to this email directly or view it on GitHub
#1275 (comment).

[LorneLaliberte]

Is there a reason into() couldn't get the context from the target? e.g. add a getContext() method to the interface (or to one that extends Target). Then it might just be a matter of something like a .themed() flag.

[JakeWharton]

It can, but what about into(Target)?

On Mon, Mar 14, 2016, 6:43 PM LorneLaliberte notifications@github.com
wrote:

Is there a reason into() couldn't get the context from the target? (e.g.
add a getContext() method to the interface)

—
Reply to this email directly or view it on GitHub
#1275 (comment).

[eneim]

Just be curious, as picasso 2 are really well formed (Really neat, I would like to say that), do you have any future plan for a fresh new release?

[LorneLaliberte]

It can, but what about into(Target)?

It would probably need to remain unthemed, unless you were willing to break compatibility by adding getContext() to the Target interface. To maintain compatibility it might make more sense to add getContext() to an interface or abstract class that extends Target instead (e.g. "ThemeableTarget"). And perhaps use something like .themed() to indicate when to bother getting the theme from the context, or to signal that a themeable target was used.

Another approach would be to do what you mentioned and add another method to pass the theme or context in, but rather than .inContext(context) which is kinda gross, using something like .themedWith(ContextThemeWrapper) or .themedWith(Theme).

[eneim]

Since I just see your 3.0 milestone, any chance this "issue" will be taken care of? Thanks.

[digitalbuddha]

One more data point, something changed between Android N and Android O theme resolution for night mode colors. We are using night mode resource loading with Picasso. Particularly we are doing

Picasso.with(context)
                .load(dimension.getUrl())
                .placeholder(R.color.image_placeholder);

With android N and below the R.color.image_placeholdervalue would resolve to our night mode color. With android O the color is always resolving to the non night color. Chasing the code down it seems to be due to picasso's request creator using the context from when the picasso instance was instantiated to resolve placeholder drawables.

private Drawable getPlaceholderDrawable() {
    if (placeholderResId != 0) {
      return picasso.context.getResources().getDrawable(placeholderResId);
    } else {
      return placeholderDrawable; // This may be null which is expected and desired behavior.
    }
  }

I'm assuming something changed in how themes are resolved in O that is causing this issue but I wasn't sure where to report

[alanv]

picasso's request creator using the context from when the picasso instance was instantiated to resolve placeholder drawables

This is WAI -- due to multi-window support, Resources objects in O+ can exist against multiple configurations within the same application.

[JakeWharton]

As of the code on master we finally have the ability to change this. We not longer require a Context for the with() call and can add overloads or new methods which take a Context for loading.

…

On Thu, Jul 20, 2017, 10:01 PM Alan Viverette ***@***.***> wrote: picasso's request creator using the context from when the picasso instance was instantiated to resolve placeholder drawables This is WAI -- due to multi-window support, Resources objects in O+ can exist against multiple configurations within the same application. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEb6Oxd12fKiO-jZm_8lq0_i8q14gks5sP8A7gaJpZM4HLI1z> .

[rekire]

I have this night theme bug in 2.8. I have an vector drawable which I tint with a theme attribute: This causes a strange behavior/bug when I use the same drawable resource id for error and placeholder. The placeholder will use the day theme and the error the dark theme.
My workaround is to use there a distince Picasso instance, but It would be nice if Picasso could use the context of the view where the image is loaded "into".

[LGFox]

I'm using picasso 3.0.0-alpha05, and this bug is still there.

I have a vector image in xml, and it uses certain colors. These colors are different in values and values-night resources. I set this vector image to .placeholder(resId). I create a new instance of Picasso (using Picasso.Builder(root.context).build()) right in the Adapter class, and this bug is still there.

If the system is using one mode (e.g. MODE_NIGHT_NO), but the app uses a different mode (e.g. MODE_NIGHT_YES) that was set using AppCompatDelegate.setDefaultNightMode(...) in Application.onCreate - this bug reproduces. Everything is OK if the system mode matches the mode in the app.

I tried using same resource id for .placeholder(resId) and .error(resId). It didn't work.

Instead of having two colors and one vector xml, I tried having two vector xml files for light and night mode each. It didn't work anyway.

The only thing that worked for me is using .placeholder(Drawable) like .placeholder(AppCompatResources.getDrawable(context, resId)).

It's almost 2024, and this bug was around for so long. Dark mode support should be de facto by now. Please, have mercy!

[LGFox]

My workaround is to use there a distince Picasso instance

Hi @rekire, could you please share your workaround?