:Margaret Leibovic Reporter
	Description • 10 years ago

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.

	:Margaret Leibovic Reporter
	Comment 1 • 10 years ago

This would be a good hack week task!

	:Margaret Leibovic Reporter
	Comment 2 • 10 years ago

I can take this.

	Lucas Rocha (:lucasr)
	Comment 4 • 10 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.

	Josh Dover [:jdover] Assignee
	Comment 5 • 10 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?

	Lucas Rocha (:lucasr)
	Comment 6 • 10 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/7b3cffd6d6c7

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 8 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/7b3cffd6d6c7