Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#8887 closed enhancement (fixed)

[patch] [cla] Augment dojox.data.GoogleSearchStore items with per-query data

Reported by: Victor Costan Owned by: Jared Jurkiewicz
Priority: high Milestone: 1.4
Component: DojoX Data Version: 1.3.0b3
Keywords: Cc:
Blocked By: Blocking:

Description

I have a patch that adds an aggregated property to each item returned by GoogleSearchStore?. The aggregated property contains attributes of the Google-produced resultData that don't belong to a specific result (and thus aren't currently copied into any item).

This is very useful (at least) in local search, where aggregated properties can help set the viewport for a map widget.

I believe the change respects the original spirit of the widget implementation -- an _aggregatedAttributes property is added to the GoogleSearchStore? class, containing the aggregation attributes, which works just like _attributes (for the per-result attributes).

I did not find any test case to modify. However, the changes are trivial, and I have tested this code in my (under development) application. I didn't find an option to upload a patch file, so I included it in the "log message" field.

I hope this change can make it in 1.3, and am willing to help make it happen.

Attachments (3)

patch.txt (7.2 KB) - added by Victor Costan 11 years ago.
(revised) patch
patch.2.txt (7.2 KB) - added by Victor Costan 11 years ago.
(revised) patch
patch.3.txt (7.2 KB) - added by Victor Costan 11 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by Victor Costan

Justification for ticket options: I filed it as a major ticket because not I believe that not having the per-query information in the Google data blocks people from doing some interesting widgets.

comment:2 Changed 11 years ago by Victor Costan

My signed CLA was processed today. Its date covers this patch. I figured I should mention this.

comment:3 Changed 11 years ago by bill

Priority: highnormal
Summary: [patch] Augment dojox.data.GoogleSearchStore items with per-query data[patch] [cla] Augment dojox.data.GoogleSearchStore items with per-query data

comment:4 Changed 11 years ago by Jared Jurkiewicz

Milestone: tbd1.4

Unfortunately, this will not make 1.3. 1.3 has been in lockdown for new features and enhancements for over a month. Only stopship/regression bugs are currently going in.

Targetting out to 1.4.

comment:5 Changed 11 years ago by Jared Jurkiewicz

I'm looking at this patch now that 1.3 is out and I have a question ... how do people access these attributes? I don't see any changes to the getValue and the like to access them .. and directly accessing them off the item handle violates the dojo.data API for how item values should be accessed.

BAD: val aggr = item.aggregatedsomeVal?;

Correct for API: val aggr = store.getValue(item, "someAggrAttr");

comment:6 Changed 11 years ago by Jared Jurkiewicz

Followup:

Submitter, can you respond to my questions? I would like to finish up and close this ticket if possible.

comment:7 Changed 11 years ago by Jared Jurkiewicz

Another followup. Submitter, are you monitoring this request?

comment:8 Changed 11 years ago by Victor Costan

Hi!

Sorry, didn't configure monitoring... I expected I'll get the comments automatically since it's my ticket.

I apologize for the patch, it doesn't work the right way, as you mentioned. I'll try to come up with something better.

Changed 11 years ago by Victor Costan

Attachment: patch.txt added

(revised) patch

comment:9 Changed 11 years ago by Victor Costan

Dear jaredj,

I have revised my patch. The previous version should have had aggregate data available via getValue(item, "aggregated"), but "aggregated" was not listed among attributes.

In any case, I revised the patch according to your suggestions, so aggregated attributes are accessible directly.

I also added a method to allow multiple parameters in the query (asides from "text" which maps to q). I believe this is essential to local searches (which are much more useful when a viewport can be specified), and will probably come handy for other stores as well.

Hope this patch is better than my first attempt. Apologies I haven't seen your messages earlier.

Changed 11 years ago by Victor Costan

Attachment: patch.3.txt added

comment:10 Changed 11 years ago by Victor Costan

There was a tiny issue with the earlier patch, I fixed it.

comment:11 Changed 11 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [17416]) Adding in CLA provided feature + cleanup of indention errors and removal of unused variable. \!strict fixes #8887

comment:12 Changed 11 years ago by Jared Jurkiewicz

(In [17417]) GoogleFeedStore? also requires small update here. \!strict refs #8887

comment:13 Changed 11 years ago by Victor Costan

Thanks, jaredj! Sorry, I totally missed the GoogleFeedStore?!

Note: See TracTickets for help on using tickets.