Closed Bug 965070 Opened 9 years ago Closed 9 years ago

Support GridViews in dynamic panels

Tracking

(Not tracked)

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 9 years ago Sola Ogunsakin [:sola] 37.39 KB, patch	lucasr : feedback+	Details \| Diff \| Splinter Review
Simplify PanelGridView 9 years ago Sola Ogunsakin [:sola] 17.65 KB, patch	lucasr : feedback+	Details \| Diff \| Splinter Review
bug-959777.patch 9 years ago Sola Ogunsakin [:sola] 17.10 KB, patch	lucasr : review+	Details \| Diff \| Splinter Review
bug-959777.patch 9 years ago Sola Ogunsakin [:sola] 17.10 KB, patch		Details \| Diff \| Splinter Review
add-constant-to-JS.patch 9 years ago Sola Ogunsakin [:sola] 850 bytes, patch	lucasr : review+	Details \| Diff \| Splinter Review

Sola Ogunsakin [:sola]

Assignee

Description

•

9 years ago

Support GridViews in dynamic panels

Sola Ogunsakin [:sola]

Assignee

Comment 1

•

9 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

•

9 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

•

9 years ago

doing a try run

:Margaret Leibovic

Comment 4

•

9 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

•

9 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

•

9 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

•

9 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

•

9 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

•

9 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

•

9 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

•

9 years ago

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

Attachment #8367664 - Attachment is obsolete: true

Sola Ogunsakin [:sola]

Assignee

Updated

•

9 years ago

Keywords: checkin-needed

Ryan VanderMeulen [:RyanVM]

Comment 12

•

9 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

•

9 years ago

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

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

Ryan VanderMeulen [:RyanVM]

Comment 14

•

9 years ago

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

Status: ASSIGNED → RESOLVED

Closed: 9 years ago

Resolution: --- → FIXED

Whiteboard: [fixed-in-fx-team]

Target Milestone: --- → Firefox 29

Lucas Rocha (:lucasr)

Comment 15

•

9 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

•

9 years ago

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

Lucas Rocha (:lucasr)

Updated

•

9 years ago

Blocks: 966022

Carsten Book [:Tomcat]

Comment 17

•

9 years ago

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

Lucas Rocha (:lucasr)

Updated

•

9 years ago

Blocks: 968179

BMO Automation

Updated

•

2 years ago

Product: Firefox for Android → Firefox for Android Graveyard

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 Graveyard :: Awesomescreen, defect)

Tracking

(Not tracked)

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

Updated