Closed Bug 963721 Opened 6 years ago Closed 6 years ago

Handle clicks on views in dynamic panels

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: Margaret, Assigned: jdover)

References

(Blocks 1 open bug)

Details

(Whiteboard: shovel-ready)

Attachments

(1 file, 1 obsolete file)

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.
This would be a good hack week task!
Whiteboard: shovel-ready
I can take this.
Assignee: nobody → margaret.leibovic
Assignee: margaret.leibovic → jdover
Attachment #8367727 - Flags: review?(lucasr.at.mozilla)
Status: NEW → ASSIGNED
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+
Attached patch patchSplinter Review
(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 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+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/7b3cffd6d6c7
Keywords: checkin-needed
Whiteboard: shovel-ready → shovel-ready[fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7b3cffd6d6c7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: shovel-ready[fixed-in-fx-team] → shovel-ready
Target Milestone: --- → Firefox 29
Blocks: 968179
Blocks: 968573
You need to log in before you can comment on or make changes to this bug.