add load image uri and make item layout responsive #24

radityarin · 2020-06-12T10:55:20Z

i try to add load image uri and make item layout responsive, please check @adityapp @gifff @zainfikrih

thanks.

adityapp

one of the reasons why we set the image type to Drawable is we want to giving flexibilities to users to choose which library they use, like Picasso or Glide. do you remember that? @gifff @zainfikrih

gridmenuview/src/androidTest/java/com/bcc/gridmenuview/MenuItemTest.java

adityapp · 2020-06-13T04:55:09Z

gridmenuview/src/main/java/com/bcc/gridmenuview/adapter/GridMenuAdapter.java

+        void bind(final MenuItem menuItem, boolean isImageUri) {
+            title.setText(menuItem.getTitle());
+            if (isImageUri) {
+                String toHttpsUri = menuItem.getImageUri().replace("http://","https://");


what do you think, should we replace http to https? which better, throw an exception to giving error message or just replace like this? @zainfikrih @gifff

gifff · 2020-06-13T07:40:42Z

Basically the issue with this implementation is that it's not scalable when you want to add more kind of image source (example: you want to load the image from local file path).

One way to solve the issue is to use generic for MenuItem and utilize Glide to load the image source.

Something like this:

public class MenuItem<T> {
    private String title;
    private T imageSource;

    public MenuItem(String title, T imageSource){
        this.title = title;
        this.imageSource = imageSource;
    }

    public String getTitle() {
        return title;
    }

    public T getImageSource() {
        return imageSource;
    }
}

and then for the adapter, you do something like this:

class ViewHolder extends RecyclerView.ViewHolder {

  // ...

    void bind(final MenuItem<?> menuItem) {
        title.setText(menuItem.getTitle());
        RequestManager glideRequest = Glide.with(itemView);
        RequestBuilder<Drawable> glideRequestBuilder = null;

        if (menuItem.getImageSource() instanceof String) {
            glideRequestBuilder = glideRequest.load(menuItem.getImageSource());
        } else  if (menuItem.getImageSource() instanceof Uri) {
            glideRequestBuilder = glideRequest.load(menuItem.getImageSource());
        } else  if (menuItem.getImageSource() instanceof Drawable) {
            glideRequestBuilder = glideRequest.load(menuItem.getImageSource());
        } else  if (menuItem.getImageSource() instanceof Bitmap) {
            glideRequestBuilder = glideRequest.load(menuItem.getImageSource());
        }

        if (glideRequestBuilder != null) {
            glideRequestBuilder.into(image);
        }

        imageOverlay.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                if (gridMenuOnClickListener != null) {
                    gridMenuOnClickListener.onClick(getAdapterPosition());
                }
            }
        });
    }
}

and the list will look like:

final ArrayList<MenuItem<?>> list = new ArrayList<>();
list.add(new MenuItem<>("satu", getResources().getDrawable(R.drawable.ic_launcher_background)));
list.add(new MenuItem<>("dua", getResources().getDrawable(R.drawable.ic_launcher_background)));
list.add(new MenuItem<>("lima", "https://cdn3.iconfinder.com/data/icons/capsocial-round/500/youtube3-128.png"));
list.add(new MenuItem<>("enam","https://cdn3.iconfinder.com/data/icons/capsocial-round/500/facebook-128.png"));

However, this library will be heavily dependent on Glide and give the users no flexibility to choose their own image loader library.

radityarin · 2020-06-13T08:25:20Z

Basically the issue with this implementation is that it's not scalable when you want to add more kind of image source (example: you want to load the image from local file path).

One way to solve the issue is to use generic for MenuItem and utilize Glide to load the image source.

Something like this:

public class MenuItem<T> {
    private String title;
    private T imageSource;

    public MenuItem(String title, T imageSource){
        this.title = title;
        this.imageSource = imageSource;
    }

    public String getTitle() {
        return title;
    }

    public T getImageSource() {
        return imageSource;
    }
}

and then for the adapter, you do something like this:

class ViewHolder extends RecyclerView.ViewHolder {

  // ...

    void bind(final MenuItem<?> menuItem) {
        title.setText(menuItem.getTitle());
        RequestManager glideRequest = Glide.with(itemView);
        RequestBuilder<Drawable> glideRequestBuilder = null;

        if (menuItem.getImageSource() instanceof String) {
            glideRequestBuilder = glideRequest.load(menuItem.getImageSource());
        } else  if (menuItem.getImageSource() instanceof Uri) {
            glideRequestBuilder = glideRequest.load(menuItem.getImageSource());
        } else  if (menuItem.getImageSource() instanceof Drawable) {
            glideRequestBuilder = glideRequest.load(menuItem.getImageSource());
        } else  if (menuItem.getImageSource() instanceof Bitmap) {
            glideRequestBuilder = glideRequest.load(menuItem.getImageSource());
        }

        if (glideRequestBuilder != null) {
            glideRequestBuilder.into(image);
        }

        imageOverlay.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                if (gridMenuOnClickListener != null) {
                    gridMenuOnClickListener.onClick(getAdapterPosition());
                }
            }
        });
    }
}

and the list will look like:

final ArrayList<MenuItem<?>> list = new ArrayList<>();
list.add(new MenuItem<>("satu", getResources().getDrawable(R.drawable.ic_launcher_background)));
list.add(new MenuItem<>("dua", getResources().getDrawable(R.drawable.ic_launcher_background)));
list.add(new MenuItem<>("lima", "https://cdn3.iconfinder.com/data/icons/capsocial-round/500/youtube3-128.png"));
list.add(new MenuItem<>("enam","https://cdn3.iconfinder.com/data/icons/capsocial-round/500/facebook-128.png"));

However, this library will be heavily dependent on Glide and give the users no flexibility to choose their own image loader library.

Thanks for the input mas gif, i'll edit the MenuItem to Generic Types.
I forgot uri means not only url but local file path is also uri too.
And for the Glide, i also thought using glide will make this library heavily dependent too, but in the end i decided to use it because CircleImageView is using library too, so i thinks its fine at the moment. So how do yo suggest, is it ok if we use Glide library or make a new classes that load image from uri?
and also how's your opinion about replacing http to https to avoid permission of cleartext http traffic? is it necessary or just let the developer who use this lib to add their own network config?
Thanks mas gif @gifff .

99ridho · 2020-06-13T08:43:51Z

@gifff solutions is also good, but if we want to add another image source type, it will require many changes on ViewHolder.

I was thought bout another solutions, we can wrap ImageSource using contract something like this. This is will give us flexibilty to decide what source that can be use to make a drawable:

interface ImageProvider {
    Drawable getDrawable();
}

public class MenuItem {
    private String title;
    private ImageProvider imageProvider;

    public MenuItem(String title, ImageProvider imageProvider){
        this.title = title;
        this.imageProvider = imageProvider;
    }

    public String getTitle() {
        return title;
    }

    public Drawable getDrawable() {
        return imageProvider.getDrawable();
    }
}

class NetworkImageProvider implements ImageProvider {

    private URL url;

    NetworkImageProvider(URL url) {
        this.url = url;
    }

    Drawable getDrawable() {
        // return drawable from url
    }
}

class LocalImageProvider implements ImageProvider {
    private String filePath;

    LocalImageProvider(String filePath) {
        this.filePath = filePath;
    }

    Drawable getDrawable() {
        // return drawable from filepath
    }
}

class AnotherImageProvider implements ImageProvider {
    Drawable getDrawable() {
        // whaterver you want
    }
}

Example:

NetworkImageProvider np = new NetworkImageProvider("https://foo.com/image.jpg");
LocalImageProvider lp = new LocalImageProvider("/media/imgs/kuda.jpg");

MenuItem item = new MenuItem("Foo", np);
MenuItem kuda = new MenuItem("Kuda", lp);

wdyt?

gifff · 2020-06-13T09:01:05Z

@radityarin

And for the Glide, i also thought using glide will make this library heavily dependent too, but in the end i decided to use it because CircleImageView is using library too, so i thinks its fine at the moment. So how do yo suggest, is it ok if we use Glide library or make a new classes that load image from uri?

Fair enough. But if we could make the image loading system pluggable, isn't it better to let the user configure the loading implementation (such as caching, transformation, etc.) ?
But to me, such implementation requires re-designing the interface.

and also how's your opinion about replacing http to https to avoid permission of cleartext http traffic? is it necessary or just let the developer who use this lib to add their own network config?

About this, I think we should leave the choices to the user. It will reduce the chance of unexpected behaviour in the user side.

@99ridho To me, it is an elegant solution 👍

radityarin · 2020-06-13T09:43:38Z

I dont know what the term is, but i think its more structured, clean and just like mas @gifff said its elegant too. Thanks mas @99ridho for the input, i'll try to implement it.

adityapp · 2020-06-13T10:15:10Z

@radityarin yes please redesign like @99ridho suggestion, I agree with that to make this project scalable

radityarin · 2020-06-13T13:51:20Z

@radityarin yes please redesign like @99ridho suggestion, I agree with that to make this project scalable

@adityapp speaking of which, this redesign is going to make current test not working, because the changes in the model MenuItem, what should i do? is it fine if i delete it first or should i edit it? thanks.

99ridho · 2020-06-13T13:59:22Z

i think refactoring is also modify the test too

adityapp · 2020-06-13T14:01:08Z

yes, if you change the contract you should change the test also

…Drawable, Local, and Network drawable that implements ImageProvider

radityarin · 2020-06-13T17:55:26Z

i try to implement mas @99ridho suggestion, please check the new commit @gifff @adityapp
thanks.

gifff · 2020-06-14T03:22:19Z

gridmenuview/src/main/java/com/bcc/gridmenuview/model/MenuItem.java

-    public MenuItem(String title, Drawable image){
+    public MenuItem(String title, ImageProvider imageProvider) {
        this.title = title;
-        this.image = image;
+        this.imageProvider = imageProvider;


Can we keep the previous constructor? That way, we don't introduce breaking-change to the previous users.

Do something like this:

public MenuItem(String title, Drawable image) { this(title, new DrawableImageProvider(image)); }

added mas @gifff , please check, thanks.

gifff · 2020-06-14T05:12:18Z

gridmenuview/src/main/res/layout/card_menu_item.xml

@@ -2,7 +2,7 @@
 <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:id="@+id/square_layout"
    android:orientation="vertical"
-    android:layout_width="wrap_content"
+    android:layout_width="match_parent"


Sorry, why this is changed?

because if the size of every icon is different, then the size of the displayed icon will be different. so i change it to match_parent, to match all the icon to same size.

match_parent

wrap_content

I see. Cool 👍

adityapp

Good jobs man, Thank you 🍻

adityapp · 2020-06-14T06:46:11Z

app/src/main/res/layout/activity_main.xml

@@ -13,4 +13,5 @@
        android:layout_height="wrap_content"
        android:layout_margin="16dp"
        app:spanCount="4" />
+
 </LinearLayout>


please add newlines after last tag

adityapp · 2020-06-14T06:57:12Z

gridmenuview/src/main/java/com/bcc/gridmenuview/task/LocalImageTask.java

+    @SuppressLint("StaticFieldLeak")
+    private Context context;
+
+    public LocalImageTask(ImageProvider imageProvider, Context context) {


I think will be better if you put context in first params to be consistency

question tho, why use @SuppressLint("StaticFieldLeak") on line 17? it just suppress the warning about the possibility of memory leaked given inside AS 🤔 . Adding WeekReference<Context> would be great solution i guess

thanks mas andri for the input, should i make a new pull request of this?

yeeep, it would be great broo @radityarin
cc: @gifff @adityapp

adityapp · 2020-06-14T07:01:24Z

gridmenuview/src/main/java/com/bcc/gridmenuview/task/NetworkImageTask.java

+    @SuppressLint("StaticFieldLeak")
+    private Context context;
+
+    public NetworkImageTask(ImageProvider imageProvider, Context context) {


will be better if the first param is context

radityarin · 2020-06-14T07:39:57Z

Good jobs man, Thank you 🍻

thanks for the review, i switched the parameter position, thanks mas @adityapp .

add load image uri and make item layout responsive (bccfilkom#24)

add load image uri and make item layout responsive

9455d66

Aldiwildan77 requested review from adityapp, gifff and zainfikrih June 12, 2020 10:57

gifff added the enhancement New feature or request label Jun 12, 2020

gifff mentioned this pull request Jun 12, 2020

add feature load image from url #25

Closed

adityapp reviewed Jun 13, 2020

View reviewed changes

fix indentation assertEquals getImageUri on MenuItemTest

f28a8f1

change Drawable in MenuItem to ImageProvider and create Provider for …

a499ad5

…Drawable, Local, and Network drawable that implements ImageProvider

gifff reviewed Jun 14, 2020

View reviewed changes

add previous constructor and modify the test

18a3162

gifff reviewed Jun 14, 2020

View reviewed changes

gifff approved these changes Jun 14, 2020

View reviewed changes

adityapp reviewed Jun 14, 2020

View reviewed changes

switch NetworkImageTask and LocalImageTask parameter position

5d8cd10

move list declaration to inside oncreate

6ce3408

adityapp merged commit d6f767d into bccfilkom:master Jun 14, 2020

gifff mentioned this pull request Jun 14, 2020

make load image from URI #20

Closed

radityarin added a commit to radityarin/gridmenu that referenced this pull request Jun 15, 2020

Merge pull request #1 from bccfilkom/master

b8e1ea8

add load image uri and make item layout responsive (bccfilkom#24)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add load image uri and make item layout responsive #24

add load image uri and make item layout responsive #24

radityarin commented Jun 12, 2020

adityapp left a comment

adityapp Jun 13, 2020

gifff commented Jun 13, 2020

radityarin commented Jun 13, 2020 •

edited

Loading

99ridho commented Jun 13, 2020

gifff commented Jun 13, 2020

radityarin commented Jun 13, 2020

adityapp commented Jun 13, 2020

radityarin commented Jun 13, 2020 •

edited

Loading

99ridho commented Jun 13, 2020

adityapp commented Jun 13, 2020

radityarin commented Jun 13, 2020

gifff Jun 14, 2020

radityarin Jun 14, 2020

gifff Jun 14, 2020

radityarin Jun 14, 2020

gifff Jun 14, 2020

adityapp left a comment •

edited

Loading

adityapp Jun 14, 2020

adityapp Jun 14, 2020

andriiginting Jun 15, 2020

radityarin Jun 16, 2020

andriiginting Jun 16, 2020

adityapp Jun 14, 2020

radityarin commented Jun 14, 2020

add load image uri and make item layout responsive #24

add load image uri and make item layout responsive #24

Conversation

radityarin commented Jun 12, 2020

adityapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gifff commented Jun 13, 2020

radityarin commented Jun 13, 2020 • edited Loading

99ridho commented Jun 13, 2020

gifff commented Jun 13, 2020

radityarin commented Jun 13, 2020

adityapp commented Jun 13, 2020

radityarin commented Jun 13, 2020 • edited Loading

99ridho commented Jun 13, 2020

adityapp commented Jun 13, 2020

radityarin commented Jun 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityapp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radityarin commented Jun 14, 2020

radityarin commented Jun 13, 2020 •

edited

Loading

radityarin commented Jun 13, 2020 •

edited

Loading

adityapp left a comment •

edited

Loading