Closed Bug 972503 Opened 6 years ago Closed 6 years ago

Allow home panel views to specify an initial filter

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: jdover, Assigned: jdover)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Once filter navigation is finished in bug 942295, we should allow views within a panel specify an initial filter. This allows multiple views and/or panels to pull from the same dataset.

{ type: Home.panels.View.LIST,
  dataset: "somedatasetid",
  initialFilter: "somefilter" }
Blocks: lists
Assignee: nobody → jdover
Attached patch patch (obsolete) — Splinter Review
I added an optional 'rootFilter' key to the view config that can be used to specify the top level filter that should be used for the retrieving the initial data from the dataset.
Attachment #8396766 - Flags: review?(lucasr.at.mozilla)
Status: NEW → ASSIGNED
Comment on attachment 8396766 [details] [diff] [review]
patch

Review of attachment 8396766 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great but I think using 'rootFilter' kinda implies hierarchy navigation which is not necessarily true i.e. the add-on dev might just want to reuse same dataset across different views in the layout by just varying the initial filter. So, let's use something simpler. Rename rootFilter to simply 'filter'. This means:

In the json: rootFilter -> filter
In HomeConfig API: ViewConfig.getRootFilter() -> ViewConfig.getFilter()

And so forth. I originally considered going with 'initialFilter' instead but I think it's implied that a filter defined in ViewConfig is the initial filter anyway. No need to be too explicit about it.
Attachment #8396766 - Flags: review?(lucasr.at.mozilla) → feedback+
Attached patch patch (obsolete) — Splinter Review
rootFilter -> filter
Attachment #8396766 - Attachment is obsolete: true
Attachment #8397268 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8397268 [details] [diff] [review]
patch

Review of attachment 8397268 [details] [diff] [review]:
-----------------------------------------------------------------

Great. Please post an updated patch.

::: mobile/android/base/home/FramePanelLayout.java
@@ +38,5 @@
>          Log.d(LOGTAG, "Loading");
>  
>          if (mChildView instanceof DatasetBacked) {
> +            DatasetRequest request = new DatasetRequest(mChildConfig.getDatasetId(),
> +                new FilterDetail(mChildConfig.getFilter(), null));

nit: maybe do:

final FilterDetail filter = new FilterDetail(mChildConfig.getFilter(), null);
final DatasetRequest request = new DatasetRequest(mChildConfig.getDatasetId(), filter);

::: mobile/android/base/home/HomeConfig.java
@@ +627,5 @@
>              validate();
>          }
>  
>          public ViewConfig(ViewType type, String datasetId, ItemType itemType,
> +                          ItemHandler itemHandler, String backImageUrl, String Filter) {

Filter -> filter
Attachment #8397268 - Flags: review?(lucasr.at.mozilla) → review+
Attached patch patchSplinter Review
Attachment #8397268 - Attachment is obsolete: true
Attachment #8398193 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/38c8dbb6ad87
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Comment on attachment 8398193 [details] [diff] [review]
patch

Note to release managers: I'm requesting uplift for a series of Firefox Hub bugs. The main fixes we need are in bugs at the end of the series, but trying to rebase those patches proved difficult and risky, so I think we should just uplift the dependecies.

Note to sheriffs: I have a local patch series that I can land on aurora when these bugs all get approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dependency for initial Firefox Hub release (promoted feed add-ons in fx30)
User impact if declined: panels won't update when data changes
Testing completed (on m-c, etc.): baked on m-c
Risk to taking this patch (and alternatives if risky): only affects dynamic panels, need an add-on to trigger this
String or IDL/UUID changes made by this patch: none
Attachment #8398193 - Flags: approval-mozilla-aurora?
Attachment #8398193 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.