Closed
Bug 963721
Opened 11 years ago
Closed 11 years ago
Handle clicks on views in dynamic panels
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 29
People
(Reporter: Margaret, Assigned: jdover)
References
Details
(Whiteboard: shovel-ready)
Attachments
(1 file, 1 obsolete file)
8.36 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
Once bug 959777 lands, we should look into how we'll handle clicks (and long clicks) on items in dynamic panels. We've talked about storing URLs with dataset items, then launching an intent with this URL when the user clicks on an item. I think this would be a good approach to start with.
Reporter | ||
Comment 2•11 years ago
|
||
I can take this.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → margaret.leibovic
Reporter | ||
Updated•11 years ago
|
Assignee: margaret.leibovic → jdover
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8367727 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 4•11 years ago
|
||
Comment on attachment 8367727 [details] [diff] [review] 0001-Bug-963721-Call-onUrlOpen-on-item-clicks-in-dynamic-.patch Review of attachment 8367727 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. Still needs a round of tweaks. ::: mobile/android/base/BrowserApp.java @@ +2483,5 @@ > public void onUrlOpen(String url, EnumSet<OnUrlOpenListener.Flags> flags) { > + if (flags.contains(OnUrlOpenListener.Flags.INTENT)) { > + Intent intent = new Intent(Intent.ACTION_VIEW); > + intent.setData(Uri.parse(url)); > + startActivity(intent); Don't we already have something in GeckoAppShell for triggering intents from URLs? Just make sure you're not duplicating code here. ::: mobile/android/base/home/HomePager.java @@ +63,5 @@ > > public interface OnUrlOpenListener { > public enum Flags { > + ALLOW_SWITCH_TO_TAB, > + INTENT INTENT -> OPEN_WITH_INTENT for clarity? ::: mobile/android/base/home/PanelListView.java @@ +25,3 @@ > > public class PanelListView extends HomeListView > + implements DatasetBacked, PanelLayout.PanelView { nit: PanelLayout.PanelView -> PanelView @@ +65,5 @@ > + private class PanelListItemClickListener implements AdapterView.OnItemClickListener { > + @Override > + public void onItemClick(AdapterView<?> parent, View view, int position, long id) { > + Cursor cursor = mAdapter.getCursor(); > + cursor.moveToPosition(position); I'd go with: if (cursor == null || !cursor.moveToPosition(position)) { throw new IllegalStateException("Couldn't move cursor to position " + position); } @@ +66,5 @@ > + @Override > + public void onItemClick(AdapterView<?> parent, View view, int position, long id) { > + Cursor cursor = mAdapter.getCursor(); > + cursor.moveToPosition(position); > + int urlIndex = cursor.getColumnIndexOrThrow(URLColumns.URL); The cursors used in PanelListView use the HomeItems schema. So this should actually be HomeItems.URL instead. nit: add an empty line above this.
Attachment #8367727 -
Flags: review?(lucasr.at.mozilla) → feedback+
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #4) Fixed up, one point of discussion: > ::: mobile/android/base/BrowserApp.java > @@ +2483,5 @@ > > public void onUrlOpen(String url, EnumSet<OnUrlOpenListener.Flags> flags) { > > + if (flags.contains(OnUrlOpenListener.Flags.INTENT)) { > > + Intent intent = new Intent(Intent.ACTION_VIEW); > > + intent.setData(Uri.parse(url)); > > + startActivity(intent); > > Don't we already have something in GeckoAppShell for triggering intents from > URLs? Just make sure you're not duplicating code here. > The only one in GeckoAppShell that would work seems much less readable to me, and is complicated (which could be more prone to breaking): > Intent intent = GeckoAppShell.getOpenURIIntent(this, url, "", Intent.ACTION_VIEW, null); It could be still more preferable to add a new static method though?
Attachment #8367727 -
Attachment is obsolete: true
Attachment #8368207 -
Flags: review?(lucasr.at.mozilla)
Comment 6•11 years ago
|
||
Comment on attachment 8368207 [details] [diff] [review] patch Review of attachment 8368207 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/home/PanelLayout.java @@ +91,5 @@ > + public interface PanelView { > + public void setOnUrlOpenListener(OnUrlOpenListener listener); > + } > + > + public PanelLayout(Context context, PanelConfig panelConfig, DatasetHandler datasetHandler, OnUrlOpenListener urlOpenListener) { Hmmm, this constructor is getting a bit too loaded but I can live with that.
Attachment #8368207 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7b3cffd6d6c7
Keywords: checkin-needed
Whiteboard: shovel-ready → shovel-ready[fixed-in-fx-team]
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7b3cffd6d6c7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: shovel-ready[fixed-in-fx-team] → shovel-ready
Target Milestone: --- → Firefox 29
Updated•4 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
•