Closed Bug 965070 Opened 6 years ago Closed 6 years ago

Support GridViews in dynamic panels

Tracking

()

Status:

RESOLVED FIXED

Milestone:

Firefox 29

People

(Reporter: oogunsakin, Assigned: oogunsakin)

References

Details

Attachments

(2 files, 3 obsolete files)

Add GridView support to dynamic panels 6 years ago Sola Ogunsakin [:sola] 37.39 KB, patch	lucasr : feedback+	Details \| Diff \| Splinter Review
Simplify PanelGridView 6 years ago Sola Ogunsakin [:sola] 17.65 KB, patch	lucasr : feedback+	Details \| Diff \| Splinter Review
bug-959777.patch 6 years ago Sola Ogunsakin [:sola] 17.10 KB, patch	lucasr : review+	Details \| Diff \| Splinter Review
bug-959777.patch 6 years ago Sola Ogunsakin [:sola] 17.10 KB, patch		Details \| Diff \| Splinter Review
add-constant-to-JS.patch 6 years ago Sola Ogunsakin [:sola] 850 bytes, patch	lucasr : review+	Details \| Diff \| Splinter Review

Sola Ogunsakin [:sola]

Assignee

Description

•

6 years ago

Support GridViews in dynamic panels

Sola Ogunsakin [:sola]

Assignee

Comment 1

•

6 years ago

Attached patch Add GridView support to dynamic panels (obsolete) — Details — Splinter Review

Attachment #8367020 - Flags: review?(lucasr.at.mozilla)

Sola Ogunsakin [:sola]

Assignee

Comment 2

•

6 years ago

Comment on attachment 8367020 [details] [diff] [review]
Add GridView support to dynamic panels

Review of attachment 8367020 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/HomeGridView.java
@@ +106,5 @@
> +        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
> +
> +        final int measuredWidth = getMeasuredWidth();
> +        if (measuredWidth == mMeasuredWidth && getCurrentRowCount() == mOldNumRows) {
> +            // Return the cached values as the width is the same.

width and height

::: mobile/android/base/home/PanelGridItemView.java
@@ +61,5 @@
> +        final String url = cursor.getString(urlIndex); */
> +
> +        try {
> +            URL url;
> +            int index = (int)(Math.random() * ((3) + 1));

will remove

Sola Ogunsakin [:sola]

Assignee

Comment 3

•

6 years ago

doing a try run

:Margaret Leibovic

Comment 4

•

6 years ago

Comment on attachment 8367020 [details] [diff] [review]
Add GridView support to dynamic panels

Review of attachment 8367020 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/AndroidManifest.xml.in
@@ +300,5 @@
>                    android:authorities="@ANDROID_PACKAGE_NAME@.db.tabs"
>                    android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"/>
>  
>          <provider android:name="org.mozilla.gecko.db.HomeProvider"
> +                  android:authorities="@ANDROID_PACKAGE_NAME@.db.home"

This fix landed with bug 964926, so you can omit this from your patch.

Lucas Rocha (:lucasr)

Comment 5

•

6 years ago

Comment on attachment 8367020 [details] [diff] [review]
Add GridView support to dynamic panels

Review of attachment 8367020 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty good but I'd like us to start with something a lot simpler than that. For instance, I don't want to start creating shared code for stuff that we're not entirely clear about in terms of design requirements. Let's do the simplest GridView (stretchMode =  STRETCH_COLUMN_WIDTH, numColumns=auto_fit, fixed item width?) with items with an image in them for now. We can iterate them as we go.

::: mobile/android/base/AndroidManifest.xml.in
@@ +300,5 @@
>                    android:authorities="@ANDROID_PACKAGE_NAME@.db.tabs"
>                    android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"/>
>  
>          <provider android:name="org.mozilla.gecko.db.HomeProvider"
> +                  android:authorities="@ANDROID_PACKAGE_NAME@.db.home"

This has been fixed in bug 964926. You can remove that from your patch.

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +11,5 @@
>  import org.mozilla.gecko.home.HomeConfig.OnChangeListener;
>  import org.mozilla.gecko.home.HomeConfig.PanelConfig;
>  import org.mozilla.gecko.home.HomeConfig.PanelType;
> +import org.mozilla.gecko.home.HomeConfig.ViewConfig;
> +import org.mozilla.gecko.home.HomeConfig.ViewType;

Unrelated, remove.

::: mobile/android/base/home/HomeGridView.java
@@ +15,5 @@
> +import android.view.View;
> +import android.widget.AbsListView;
> +import android.widget.GridView;
> +
> +

nit: remove extra empty line here.

@@ +19,5 @@
> +
> +/**
> + * Base GridView for HomePanels.
> + */
> +public abstract class HomeGridView extends GridView {

We don't know yet if we'll need the same kind of measurement logic for the PanelGridView too. So, for now, let's simply use a plain GridView for PanelGridView with the shared HomeGridView style that you created.

::: mobile/android/base/home/PanelGridItemView.java
@@ +26,5 @@
> +import android.widget.RelativeLayout;
> +import android.widget.TextView;
> +
> +
> +public class PanelGridItemView extends RelativeLayout {

Make it a FrameLayout for now.

@@ +64,5 @@
> +            URL url;
> +            int index = (int)(Math.random() * ((3) + 1));
> +            url = new URL("http://farm4.staticflickr.com/3566/3362714321_65c818f439_q.jpg");
> +            Bitmap bmp = BitmapFactory.decodeStream(url.openConnection().getInputStream());
> +            mThumbnailView.setImageBitmap(bmp);

You can leave this debugging code out. The image loading stuff for this and other panel views will be implemented in bug 963046.

::: mobile/android/base/home/PanelGridView.java
@@ +36,5 @@
> +    public void setDataset(Cursor cursor) {
> +        mAdapter.swapCursor(cursor);
> +    }
> +
> +    public class PanelGridViewAdapter extends CursorAdapter {

This should be private.

nit: declare private inner classes at the end of PanelGridView

::: mobile/android/base/home/TopSitesGridView.java
@@ +168,5 @@
> +	}
> +
> +	@Override
> +	protected View getChildView() {
> +		return new TopSitesGridItemView(getContext());

Why is this necessary? This is creating an instance of  on each onMeasure() call which will lead of a lot of objects being created unnecessarily.

::: mobile/android/base/home/TopSitesPanel.java
@@ +30,5 @@
>  import android.content.Context;
>  import android.content.Intent;
>  import android.content.res.Configuration;
>  import android.database.Cursor;
> +import android.database.DatabaseUtils;

Unrelated, remove.

::: mobile/android/base/resources/layout/panel_grid_item_view.xml
@@ +9,5 @@
> +    <org.mozilla.gecko.home.TopSitesThumbnailView
> +            android:id="@+id/thumbnail"
> +            android:layout_width="fill_parent"
> +            android:layout_height="wrap_content"
> +            android:layout_alignParentTop="true"/>

Use the more general ThumbnailView here.

@@ +11,5 @@
> +            android:layout_width="fill_parent"
> +            android:layout_height="wrap_content"
> +            android:layout_alignParentTop="true"/>
> +
> +    <org.mozilla.gecko.home.FadedTextView

No need to add title for now. Let's do the simples possible item to start with.

Attachment #8367020 - Flags: review?(lucasr.at.mozilla) → feedback+

Sola Ogunsakin [:sola]

Assignee

Comment 6

•

6 years ago

Attached patch Simplify PanelGridView (obsolete) — Details — Splinter Review

Attachment #8367020 - Attachment is obsolete: true

Attachment #8367596 - Flags: review?(lucasr.at.mozilla)

Sola Ogunsakin [:sola]

Assignee

Comment 7

•

6 years ago

Comment on attachment 8367596 [details] [diff] [review]
Simplify PanelGridView

Review of attachment 8367596 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/PanelGridView.java
@@ +35,5 @@
> +    }
> +
> +    @Override
> +    public void setDataset(Cursor cursor) {
> +    	Log.d(LOGTAG, DatabaseUtils.dumpCursorToString(cursor));

will remove

Lucas Rocha (:lucasr)

Comment 8

•

6 years ago

Comment on attachment 8367596 [details] [diff] [review]
Simplify PanelGridView

Review of attachment 8367596 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, just needs one more round of tweaks.

::: mobile/android/base/home/PanelGridItemView.java
@@ +36,5 @@
> +        this(context, null);
> +    }
> +
> +    public PanelGridItemView(Context context, AttributeSet attrs) {
> +        this(context, attrs, R.attr.topSitesGridItemViewStyle);

Just skip the style id for now.

@@ +40,5 @@
> +        this(context, attrs, R.attr.topSitesGridItemViewStyle);
> +    }
> +
> +    public PanelGridItemView(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);

nit: add empty line here.

@@ +41,5 @@
> +    }
> +
> +    public PanelGridItemView(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        LayoutInflater.from(context).inflate(R.layout.panel_grid_item_view, this);

nit: remove empty line here :-)

::: mobile/android/base/home/PanelGridView.java
@@ +19,5 @@
> +import android.view.ViewGroup;
> +import android.widget.GridView;
> +
> +public class PanelGridView extends GridView
> +                    implements DatasetBacked {

Align the 'implements' with the 'extends'.

@@ +22,5 @@
> +public class PanelGridView extends GridView
> +                    implements DatasetBacked {
> +    private static final String LOGTAG = "GeckoPanelGridView";
> +
> +    private final ViewConfig mViewConfig;

Not using it, remove.

@@ +26,5 @@
> +    private final ViewConfig mViewConfig;
> +    private final PanelGridViewAdapter mAdapter;
> +
> +    public PanelGridView(Context context, ViewConfig viewConfig) {
> +        super(context, null, R.attr.topSitesGridViewStyle);

Didn't you mean homeGridViewStyle here?

@@ +47,5 @@
> +        }
> +
> +        @Override
> +        public void bindView(View bindView, Context context, Cursor cursor) {
> +            final PanelGridItemView row = (PanelGridItemView) bindView;

row -> item

::: mobile/android/base/home/PanelLayout.java
@@ +9,5 @@
>  import org.mozilla.gecko.home.HomeConfig.ViewConfig;
>  
>  import android.content.Context;
>  import android.database.Cursor;
> +

Unrelated, remove.

::: mobile/android/base/resources/layout/panel_grid_item_view.xml
@@ +5,5 @@
> +
> +<merge xmlns:android="http://schemas.android.com/apk/res/android"
> +       xmlns:gecko="http://schemas.android.com/apk/res-auto">
> +
> +    <ImageView android:id="@+id/thumbnail"

thumbnail -> image (for consistency with our dataset schema)

@@ +8,5 @@
> +
> +    <ImageView android:id="@+id/thumbnail"
> +               android:layout_width="fill_parent"
> +               android:layout_height="wrap_content"
> +               android:src="@drawable/top_site_add" />

nit: add empty line here.

Also, remove the 'top_site_add' image reference for now.

Attachment #8367596 - Flags: review?(lucasr.at.mozilla) → feedback+

Sola Ogunsakin [:sola]

Assignee

Comment 9

•

6 years ago

Attached patch bug-959777.patch (obsolete) — Details — Splinter Review

Attachment #8367596 - Attachment is obsolete: true

Attachment #8367664 - Flags: review?(lucasr.at.mozilla)

Lucas Rocha (:lucasr)

Comment 10

•

6 years ago

Comment on attachment 8367664 [details] [diff] [review]
bug-959777.patch

Review of attachment 8367664 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome. Maybe post another patch with the nits fixed for landing?

::: mobile/android/base/home/PanelGridItemView.java
@@ +26,5 @@
> +import android.widget.ImageView.ScaleType;
> +import android.widget.FrameLayout;
> +import android.widget.TextView;
> +
> +

nit: remove extra empty line here.

::: mobile/android/base/resources/layout/panel_grid_item_view.xml
@@ +8,5 @@
> +
> +    <ImageView android:id="@+id/image"
> +               android:layout_width="fill_parent"
> +               android:layout_height="80dp"
> +               android:layout_marginRight="5dp" />

nit: add empty line here.

Attachment #8367664 - Flags: review?(lucasr.at.mozilla) → review+

Sola Ogunsakin [:sola]

Assignee

Comment 11

•

6 years ago

Attached patch bug-959777.patch — Details — Splinter Review

Attachment #8367664 - Attachment is obsolete: true

Sola Ogunsakin [:sola]

Assignee

Updated

•

6 years ago

Keywords: checkin-needed

Ryan VanderMeulen [:RyanVM]

Comment 12

•

6 years ago

https://hg.mozilla.org/integration/fx-team/rev/0935601c8611

Keywords: checkin-needed

Whiteboard: [fixed-in-fx-team]

Sola Ogunsakin [:sola]

Assignee

Comment 13

•

6 years ago

Attached patch add-constant-to-JS.patch — Details — Splinter Review

Attachment #8368016 - Flags: review?(lucasr.at.mozilla)

Ryan VanderMeulen [:RyanVM]

Comment 14

•

6 years ago

https://hg.mozilla.org/mozilla-central/rev/0935601c8611

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

Resolution: --- → FIXED

Whiteboard: [fixed-in-fx-team]

Target Milestone: --- → Firefox 29

Lucas Rocha (:lucasr)

Comment 15

•

6 years ago

Comment on attachment 8368016 [details] [diff] [review]
add-constant-to-JS.patch

Review of attachment 8368016 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Attachment #8368016 - Flags: review?(lucasr.at.mozilla) → review+

Lucas Rocha (:lucasr)

Comment 16

•

6 years ago

https://hg.mozilla.org/integration/fx-team/rev/46c932e276ea

Lucas Rocha (:lucasr)

Updated

•

6 years ago

Blocks: 966022

Carsten Book [:Tomcat]

Comment 17

•

6 years ago

https://hg.mozilla.org/mozilla-central/rev/46c932e276ea

Lucas Rocha (:lucasr)

Updated

•

6 years ago

Blocks: 968179

You need to log in before you can comment on or make changes to this bug.

Bugzilla

Support GridViews in dynamic panels

Categories

(Firefox for Android :: Awesomescreen, defect)

Tracking

()

People

(Reporter: oogunsakin, Assigned: oogunsakin)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(2 files, 3 obsolete files)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Updated

Comment 12

Comment 13

Comment 14

Comment 15

Comment 16

Updated

Comment 17

Updated