Closed
Bug 965070
Opened 9 years ago
Closed 9 years ago
Support GridViews in dynamic panels
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 29
People
(Reporter: oogunsakin, Assigned: oogunsakin)
References
Details
Attachments
(2 files, 3 obsolete files)
17.10 KB,
patch
|
Details | Diff | Splinter Review | |
850 bytes,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
Support GridViews in dynamic panels
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8367020 -
Flags: review?(lucasr.at.mozilla)
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
Assignee | ||
Comment 3•9 years ago
|
||
doing a try run
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.
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+
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8367020 -
Attachment is obsolete: true
Attachment #8367596 -
Flags: review?(lucasr.at.mozilla)
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
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+
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8367596 -
Attachment is obsolete: true
Attachment #8367664 -
Flags: review?(lucasr.at.mozilla)
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+
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8367664 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0935601c8611
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8368016 -
Flags: review?(lucasr.at.mozilla)
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
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+
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.
Description
•