Closed Bug 949039 Opened 6 years ago Closed 6 years ago

Improve queryFakeItems to return more data, including different types of list items

Categories

(Firefox for Android :: Data Providers, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

(Whiteboard: shovel-ready)

Attachments

(1 file, 3 obsolete files)

To make it easy to develop the UI bits of the list work, we should create a way to get a Cursor with a bunch of test data.
Bug 941318 took care of adding a "fake" data endpoint, but we should flesh it out more to return different kinds of data.

We also discussed pulling this data from a JSON file, perhaps letting a test add-on specify which file to use to get that data.
No longer blocks: 941318
Depends on: 941318
Summary: Create an endpoint for list content provider that returns static test data → Improve queryFakeItems to return more data, including different types of list items
Assignee: margaret.leibovic → nobody
Whiteboard: shovel-ready
Assignee: nobody → margaret.leibovic
This renames the PROVIDER_ID column to LIST_ID, to make it more clear that we can have more than one list per provider.
Attachment #8357508 - Flags: review?(wjohnston)
This patch lets us pull fake items from a JSON file. I wrapped this extra file in nightly build defines, since we wouldn't want to ship this file.

In a future patch we can add more data to this file, but this will also make it easier for developers to add their own things to this file (although they'd still need to update the Java if they want more columns).

Also, I modeled this after recommended-addons.json, but I filed bug 957881 to remove that, since I noticed I don't think we use that file anymore.

And I still need to write to let us actually filter these list items per list id.
Attachment #8357510 - Flags: review?(wjohnston)
(In reply to :Margaret Leibovic from comment #2)
> Created attachment 8357508 [details] [diff] [review]
> (Part 1) Rename PROVIDER_ID to LIST_ID
> 
> This renames the PROVIDER_ID column to LIST_ID, to make it more clear that
> we can have more than one list per provider.

(In my mind, "provider" means a data provider that chooses to ship an add-on, such as a news provider shipping an add-on with multiple lists for different news sections.)
Comment on attachment 8357510 [details] [diff] [review]
(Part 2) Pull fake list items from a JSON file

I just realized that this will cause testHomeListsProvider to fail when it hits aurora... maybe I should just get rid of that test for fake items. Or maybe I can put that behind a build flag as well.
Comment on attachment 8357508 [details] [diff] [review]
(Part 1) Rename PROVIDER_ID to LIST_ID

Don't want this patch anymore.
Attachment #8357508 - Attachment is obsolete: true
Attachment #8357508 - Flags: review?(wjohnston)
Comment on attachment 8357510 [details] [diff] [review]
(Part 2) Pull fake list items from a JSON file

Downgrade to feedback... I want to update this based on the discussion in our meeting this morning.
Attachment #8357510 - Flags: review?(wjohnston) → feedback?(wjohnston)
Given that we've still been discussing how we'll do the real backend storage here, let's punt on renaming things just yet, but I'd like to land this patch, since it will make it easier for developers to hack on the UI views without waiting for the backend bits to work.

To test this, I just threw this line of code inside HomeConfigPrefsBackend.loadDefaultConfig():

panelConfigs.add(new PanelConfig(PanelType.DYNAMIC, "Dynamic", dynamic"));

This works because right now DynamicPanel just creates a ListView that's backed by a Cursor returned from queryFakeItems. This will change as lucasr works in bug 959777 to make DynamicPanel actually do what it's supposed to do, but even if that code lands, a developer could easily plug in whatever views they want to try to test (ideally, the could just create them with a PanelConfig).
Attachment #8357510 - Attachment is obsolete: true
Attachment #8357510 - Flags: feedback?(wjohnston)
Attachment #8360790 - Flags: review?(wjohnston)
As part of this patch, I also think we should just disable testHomeListsProvider. It's not actually doing much of anything useful right now, so I don't want to invest in updating it until we change HomeListsProvider around to be more useful (bug 961092).
Comment on attachment 8360790 [details] [diff] [review]
Pull fake list items from a JSON file (without renaming things)

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

Nice. I think you can simplify it a little actually though :) yay!?

::: mobile/android/base/db/HomeListsProvider.java
@@ +415,5 @@
> +    private String getRawFakeItems() throws IOException {
> +        final ZipFile zipFile = new ZipFile(getContext().getPackageResourcePath());
> +        final ZipEntry zipEntry = zipFile.getEntry("fake-list-items.json");
> +
> +        final InputStream inputStream = zipFile.getInputStream(zipEntry);

It should be simpler to just ship this as a raw resource. i.e. add a raw folder in mobile/android/base/resources. I think our build will just move it for you now (thanks nalexander!), and then use:

InputStream inputStream = getResources().openRawResource(R.raw.fake_list_items);
Attachment #8360790 - Flags: review?(wjohnston) → review-
Nice suggestion :)

I think the robocop test was failing before just because I renamed something without updating the test. It passes locally for me now, and I pushed to try to verify:
https://tbpl.mozilla.org/?tree=Try&rev=371d82f82324

As much as this test isn't useful in that it isn't testing code we ship, it is useful to test this content provider endpoint, and to make sure it doesn't get broken.
Attachment #8360790 - Attachment is obsolete: true
Attachment #8364037 - Flags: review?(wjohnston)
Blocks: 952310
Attachment #8364037 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/a1f08c3b158a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.