Review 3 of aggregator (alpha 2)
| Project: | New Aggregator for Drupal core |
| Version: | 7.x-0.1-alpha2 |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Jump to:
Aron - this list is pretty much in the order I came across things:
1) Use "category" instead of "terms"
I committed a fix to this (in wording and paths) - but I'm sure I haven't hit all items...
2) Deduper shouldn't be on UI
Let's chat about this... I haven't wrapped my mind around this issue, but I don't want to bother admins (think: beginner site builders) with decisions like which deduper to use.
3) $source_image and $source_description not working in aggregator-feed-source.tpl.php
4) Do we still need aggregator-feed-source.tpl.php given the fact that a feed is a node now?
5) Hypothetical question: Could we override settings on a per feed basis if necessary?
If an 3rd party module wanted to, could it override settings on a per feed basis?
6) Don't show advanced configuration options field set if there are none.
7) Don't show categories menu if there are no categories
8) Break out all aggregator light functionality into a aggregator_light.inc and only load if needed.
The goal is here to isolate what's not part of the aggregator API, to put into an .inc file what 'could' be a module at one point and to not load code when it's not necessary.
9) Break out deletion of expired feed items into its own function and stick it into aggregator_light.inc
10) light, light, light... I'm still not happy with that name... probably we get to something better. let's keep it until then.
11) Update interval: 'none' is missing
There should be the choice 'none' for a feed content type - think of building a feed processor for lazy instantiation.
12) Use description in parser/processor UI
13) Compare aggregator/sources/1 between old and new aggregator
On old aggregator there was a 'categorize' tab and a 'configure' tab. we should use the standard node/1 page instead of aggregator/sources/1 - i know that this will collapse the menu tree, but that's the best idea i have for now. this means that a node view of a light aggregator feed will be a listing. we should chat about this.
14) when i create a new content type, there is no 'feed aggregator' tab on it
15) make "Is a feed content type" a button "Would you like to use this content type for downloading feeds to your site?" "Enable" or "Disable" when disabled, don't show form - cleaner.
16) Don't cascade collapsible forms: a collapsible form for aggregator light within a collapsible form for all processors is not necessary. just skip the collapsible form for advanced settings for all processors and the collapsible form for advanced settings for all parsers.
#1
The below list of things were done:
- Deduper is only a variable, but not a UI element (so advanced users only need to alter the DB, not the code if want to change deduper)
- source_image and source_icon works
- Help text added to category/terms (so it's not empty even if there are no categories), maybe language improvements and more verbosity is needed here
- Description is added to Feed aggregator tab / processor/parser settings
- No nested collapsible forms at Feed aggregator tab
- Refresh interval: none is added, you can turn off feed refresh, per-content-type
- All Aggregator Light related code now live in aggregator.light.inc
#2
Thanks for response.
2) Overridable deduping: There shouldn't even be a variable. It's not necessary: the last module called on 'unique' should override any previous returns of 'unique', aggregator should respect the module's weight when calling the hook_process('unique') callback.
#3
Hm. I don't really see how this could work.
foreach ($processors as $processor) {if (count($processors) == 1 && $deduper != FALSE) {
module_invoke($deduper, 'aggregator_process', 'unique', $node);
}
else {
module_invoke($processor, 'aggregator_process', 'unique', $node);
}
$items = module_invoke($processor, 'aggregator_process', 'save', $node);
You could see how does this work now. Using 'last' module 'unique' implementation doesn't seem to be appropriate. Because in valid (and no overrideable deduping configuration) processor configurations all of the processors use their own 'unique'. How aggregator could detect that now there is an overrided deduping.
#4
1) Use "category" instead of "terms"
Disagree. I absolutely agree that we should use the same words for the same
things! But the whole "categorization" in the new aggregator is totally
different, it's based on taxonomy, as you know. In this case I think we
should use taxonomy words. It shows the user: something is changed with this
and it uses taxonomy because of using the same words like in taxonomy.
2) Deduper shouldn't be on UI
Agree. See my other comment about how it should be exactly done.
3) $source_image and $source_description not working in
aggregator-feed-source.tpl.php
Done.
4) Do we still need aggregator-feed-source.tpl.php given the fact that a feed
is a node now?
Yes, we need. (it really depends on about the decision at 13) )
5) Hypothetical question: Could we override settings on a per feed basis if
necessary?
Currently there is no way to do.
6) Don't show advanced configuration options field set if there are none.
Done.
7) Don't show categories menu if there are no categories
I put a helper text in this case (no terms) on the page: how the term
system works and how you could use terms and see feed items in it)
10) light, light, light.. If anybody has a bettet name idea for Aggregator Light, please share it here :)
8-12 (exculde 10) )
Done.
13) Compare aggregator/sources/1 between old and new aggregator
This is a huge change (for the user part). I agree that every apsect of the feed should be handled from one place. This question needs more thinking and concepting.
14) when i create a new content type, there is no 'feed aggregator' tab on it
Fixed. (because of hypen - underscore conversion in the URL)
15) Make "Is a feed content type" a button
More concepting is needed. It's all about (+ 13) also ) intuitive user interface, really hard questions.
16) No nested collapsible stuff.
Done
#5
How about that?
foreach ($processors as $processor) {module_invoke($processor, 'aggregator_process', 'unique', $node);
}
foreach ($processors as $processor) {
$items = module_invoke($processor, 'aggregator_process', 'save', $node);
}
#6
#5 Moved to: http://drupal.org/node/285427