Closed Bug 965070 Opened 8 years ago Closed 8 years ago

Support GridViews in dynamic panels

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

Product:
Firefox for Android Graveyard
Component:
Awesomescreen
Platform:
x86
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 29

People

(Reporter: oogunsakin, Assigned: oogunsakin)

References

Details

Attachments

(2 files, 3 obsolete files)

bug-959777.patch
17.10 KB, patch
Details | Diff | Splinter Review
add-constant-to-JS.patch
850 bytes, patch
lucasr
: review+
Details | Diff | Splinter Review 
Support GridViews in dynamic panels
Attachment #8367020 - Flags: review?(lucasr.at.mozilla)
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
doing a try run
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.
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+
Attached patch Simplify PanelGridView (obsolete) — — Splinter Review
Attachment #8367020 - Attachment is obsolete: true
Attachment #8367596 - Flags: review?(lucasr.at.mozilla)
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
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+
Attached patch bug-959777.patch (obsolete) — — Splinter Review
Attachment #8367596 - Attachment is obsolete: true
Attachment #8367664 - Flags: review?(lucasr.at.mozilla)
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+
Attached patch bug-959777.patch — — Splinter Review
Attachment #8367664 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8368016 - Flags: review?(lucasr.at.mozilla)
https://hg.mozilla.org/mozilla-central/rev/0935601c8611
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
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+
Blocks: 966022
Blocks: 968179
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.