I would like to kill this at once.
I did an initial cleanup.

Current UI situation: http://awesomescreenshot.com/0ba6awv12
Overkill of text and options/buttons/textfields.

My suggestion:
views style: http://awesomescreenshot.com/0b96awgdf
views row style: http://awesomescreenshot.com/08a6awr61

- I alrdy fixed the hiding of the date field :D.
- Actually don't like the fieldsets in this context, I don't now if we can have the labels, but not the borders.
- I WANT THE FREAKING DESCRIPTION AFTER MY TITLE :D

Comments? Anyone? :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Priority: Normal » Major

If we're removing the autodiscovery, then the date field needs to be specified.

aspilicious’s picture

I think I'm done...
I changed a lot of code.

Small summary:
- Views UI cleanup
- Remove autodiscovery
- Allow the use of fields
==> for every date field you select the date will be added to the calendar.

NEW POSIBILITIES:
* Split an event in multiple time periods, they all will be added.
* Select only the date fields you would like to see on the calender. == more control of the view

- fixed title/url replace == nize!

TODO:
- Tweak the UI a little more like I said before, can be a new issue.
- If the date field is empty there are errors (also before this patch) ==> Make a new issue?

aspilicious’s picture

Quick code review of my own patch.
Now I'm done, other people can review/edit.

+++ fullcalendar.install
@@ -37,7 +37,7 @@
 /**
- * Implementation of hook_uninstall().
+ * Implements of hook_uninstall().
  */

This sneaked in and is still wrong, of needs to be removed

+++ fullcalendar.module
@@ -230,40 +235,41 @@
-      // If a date_type field exists
-      if (isset($display_field[$field_name])) {
-        $display_field = array($field_name => $display_field[$field_name]);
-        break;
-      }

We need some kind of check if there are dates to display and check if they are empty :)
The removed code isn't correct anymore, just putting it here for referenc.

+++ fullcalendar.module
@@ -360,21 +366,13 @@
- * field_info_field() to find that.
+ * @param $fields
+ *   Array of possible date fields.
  */

I renamed the param in the code

Powered by Dreditor.

aspilicious’s picture

Fixed all the things in the previous review.

TODO:
Fix update, isn't working anymore... :(

tim.plunkett’s picture

Status: Active » Needs work
FileSize
6.96 KB
20.18 KB

We can't use $node->language like that, there are per-field language settings as well.
I want to completely destroy autodiscovery. They should be forced to pick a date field.

Here are other tweaks. Attached is the whole patch, and an innerdiff of this and #4. It doesn't make any real changes.

aspilicious’s picture

"We can't use $node->language like that, there are per-field language settings as well."
Fixed :D

"I want to completely destroy autodiscovery. They should be forced to pick a date field."
Thats is alrdy done? Or isn't it? Me confused...
I *ONLY* use the fields that are specified in the views UI.
Can you explain a little more what you expect?

aspilicious’s picture

There where some warnings and errors when we didn't select any date field.
Caused by a non initialised array.

aspilicious’s picture

Status: Needs work » Needs review

Needs review... I guess?

tim.plunkett’s picture

Status: Needs review » Needs work

Why not have a custom date selection like the URL and Title, in case two date fields are added?

Views throws errors if no fields are added. Perhaps we should throw an error if no date field is added?

If those two get addressed, this is pretty close to awesome :)

aspilicious’s picture

But what If I would Like to add two date fields for one event. I have use cases for that. I have an event that is split into two "time frames".

AWSOME EVENT
---------------
Date first frame:
Sunday, January 23, 2011 - 13:00 - 19:00
Datum second frame:
Tuesday, January 25, 2011 - 21:56 - Thursday, January 27, 2011 - 21:56

The current implementation supports that and just doesn't show any events when there are none defined. And if you only want one of the two "time frames" displayed, throw the second field out of your field list. Simple as that...
I made 3 different calendars with the same data.

An error is a little hard or not? But maybe we can add a status message above the calendar??
"No date fields are selected"

EXTRA: attached patch fixes the drag and drop saving, base path issue

TODO (just for reference):
When this is fixed we should pay attention to one problem:
1) Apparantly IE7 doens't display the calendar :(

aspilicious’s picture

Status: Needs work » Needs review
aspilicious’s picture

New version

tim.plunkett’s picture

Status: Needs review » Needs work
+++ fullcalendar.module
@@ -230,49 +237,58 @@
+    $title_field_as_entity = $vars['options']['fullcalendar_title_field'];
+    $title_field = $fields[$title_field_as_entity];

Could just be $title_field = $fields[$vars['options']['fullcalendar_title_field']];

+++ fullcalendar.module
@@ -230,49 +237,58 @@
+    $title_field_value = $node->{$title_field}[$language][0]['value'];
+    if (!empty($title_field_value)) {
+      $node->title = $title_field_value;
     }

No need for the extra assignment. Similar to above.

+++ fullcalendar.module
@@ -230,49 +237,58 @@
+  if(!empty($vars['options']['fullcalendar_custom_url'])) {
+    $url_field_as_entity = $vars['options']['fullcalendar_url_field'];
+    $url_field = $fields[$url_field_as_entity];
+    $language = field_language($entity_type, $node, $url_field);
+    $url_field_value = $node->{$url_field}[$language][0]['value'];
+    if (!empty($url_field_value)) {
+      $node->url = $url_field_value;

Same as title section.

+++ fullcalendar.module
@@ -230,49 +237,58 @@
+  ¶
+  // Fetch custom dates if needed.
+  if(!empty($vars['options']['fullcalendar_custom_date'])) {
+  	$date_field_entities = $vars['options']['fullcalendar_date_field'];
+  	$fields = array_intersect_key($fields, $date_field_entities);
+  }
+  ¶

Trailing spaces, tabs. And unneeded assignment.

+++ fullcalendar.module
@@ -230,49 +237,58 @@
+    if(!empty($items)) {

Space between if ()

+++ fullcalendar.module
@@ -360,24 +376,20 @@
+ * @param $view_fields
+ *   Array of possible date fields.
  */
-function fullcalendar_date_fields($entity, $entity_type = 'node') {

Document $field_names. Perhaps an inline comment as well

+++ fullcalendar.views.inc
@@ -52,12 +52,12 @@
-        'title' => t('Node - FullCalendar'),
+        'title' => t('Fields - FullCalendar'),

The dash with spaces always bugged me. How about 'Fields (FullCalendar)'?

+++ views_plugin_node_fullcalendar.inc
@@ -19,30 +19,62 @@
+    $options['fullcalendar_date_field'] = array('default' => '');

No reason to rename this. Especially since $date_fields is used below.

+++ views_plugin_style_fullcalendar.inc
@@ -64,20 +62,19 @@
+      '#default_value' => t($this->options['display']['fc_firstday']),

Wasn't this just wrapped in t() above?

Powered by Dreditor.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
21.18 KB

Last reroll? :)

aspilicious’s picture

Timplunkett I think you were right. When I remove every node I get this:
Notice: Undefined property: stdClass::$nid in template_preprocess_views_view_node_fullcalendar()
So could you put the code back preventing this issue?

EDIT:
Tested it locally, it works when you put it back.

tim.plunkett’s picture

Amazing work aspilicious!

I cleaned up some of the code, and I think I fixed the multiselect.

Regular patch is against a clean branch, innerdiff is against code patched with #14.

aspilicious’s picture

LOL, you're to fast :(. This morning I woke up and said to myself, stupid idiot you forgot the select process in the multiselect...
I run to my laptop and I see your commit...

:D

I tested a gazillions things and I think we are safe...
I would like to RTBC this but I feel like I'm not the person marking this RTBC.
Oh btw, shouldn't we open a 7.2 branch?

tim.plunkett’s picture

We really have the benefit of being in two different timezones, it's like there's always someone hacking on fullcalendar at all times!

Maybe geerlingguy can test it.

And yeah, this would be the first commit to the 7.2 branch.

geerlingguy’s picture

In my testing, I couldn't select any fields - even after clearing caches. Then, after having cleared said caches, no events showed on my fullcalendar (was working fine before patch).

[Edit: D'oh! I didn't notice that I had to first add the field to the view, then select the field from the select area. Just a matter of documentation, really. We should maybe put a little notice at the top of the Field Settings pane in the View that says "Fields must be added to the view before they are available to select here." (something like that).

But it's up to timplunkett - otherwise, the patch works flawlessly, and I'd RTBC. Either put in the description or not, but it's practically ready for commit.

tim.plunkett’s picture

Version: 7.x-1.x-dev » 6.x-2.x-dev
Status: Needs review » Patch (to be ported)
FileSize
23.19 KB

Committed to DRUPAL-7--2.
http://drupal.org/cvs?commit=489546

Needs backport.

Here's the final patch I committed, with string tweaks.

aspilicious’s picture

Status: Patch (to be ported) » Needs review
FileSize
18.7 KB

Needs #1035246: Incompatible with Views 6.x-3.x

Ok first try :) (btw I hate D6)

TODO
-----
- the ctools hiding stuff doesn't work anymore
- disabled custom date field cause it's broken
- had to make 2 extra functions => review

aspilicious’s picture

You broke the custom settings in your last patch. I missed that on my final review.

Here is a D7 followup patch. (found it while working on the D6 stuff)

tim.plunkett’s picture

Status: Needs review » Needs work

#21,
Views doesn't depend on CTools in D6, so we can't expect them to have it. Instead of '#process' => array('ctools_dependent_process'), it's '#process' => array('views_process_dependency'). The #dependency line should be fine.
I'll look at the other stuff later (those new functions look unfortunate), but you might as well roll in the option renaming from #22. And mind your tabs!

#22,
Yeah, whoops. Committing.

aspilicious’s picture

I need to do something about those tabs...
Can't manage to configure my IDE well enough...
+
Dreditor is broken on latest greasemonkey :(

#22 is alrdy in there.

Lets find those damn tabs...

aspilicious’s picture

Status: Needs work » Needs review
FileSize
19.44 KB

Still tabs in it

aspilicious’s picture

Without tabs *I hope*.
And now only selected fields will be displayed.

aspilicious’s picture

Another try...

aspilicious’s picture

Investigating an other way to fix this, so we can remove the node load dependency...

aspilicious’s picture

Ok got some interesting results.

1) I copied the workflow from D7. ==> we don't need the two extra functions anymore! Yeah!
2) I only strip the _value in the end ==> Yihaa!

3) Apparantly the value of the field is attached to the node. It is impossible to fetch the value of a field without loading the node. Or I couldn't find it ==> NOOOOO

==> we are looking for a field_get_items but that doesn't rly. Or does it? :)

aspilicious’s picture

wtf thats not the correct patch...

aspilicious’s picture

Better?

Srry for the overload off patches :)

aspilicious’s picture

After investigation, I think it is safe to give this a serious review. We need some more investigation how to delete the node->load call but that must not prevent this initial patch to be committed. We are not rdy for a stable release so we have still time to search for a solution. But I would like to deal with that in an other issue.

OK?

aspilicious’s picture

Can someone give this a review? I would like to port all the other patches but they need a clean views UI :).

corey.aufang’s picture

Assigned: aspilicious » Unassigned

Patch in #1036444-31: Cleanup the views UI + remove autodiscovery mega issue. breaks at line 37 of views_plugin_node_fullcalendar.inc

The data in field_options does not match what is returned by fullcalendar_date_fields.

You can see whats supposed to match, but the data returned by get_field_labels appends "_value" to cck fields.

aspilicious’s picture

It don't break at my installation...
can you try a new view?

corey.aufang’s picture

I mean it breaks in the sense that when you choose row style "Fields (FullCalendar)" and select "Use a custom date field." The field list is empty.

aspilicious’s picture

- Read the instructions of version 2: http://drupal.org/node/1056742#usage

1) You NEED to add a field first.
==> There is also a warning on the page: 'Add fields to the view in order to customize fields below.',

2) custom date fields are optional, if you do not select one it will use all the date fields added in the views UI.

corey.aufang’s picture

Opps, I wasn't applying the patch to the dev version. :/

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Even though D6 is boring to me now, I know I should review this.
Actually, someone else should review it, and I should look over it after that.... but yeah, assigning to myself.

tim.plunkett’s picture

This merges in #1049028: Remove theme_fullcalendar_link, I couldn't get it to work without that.

aspilicious’s picture

Assigned: Unassigned » aspilicious
FileSize
1.39 KB

Default date thingie was broken.
A diff fix attached.

Default date starting with week view or day view doesn't give the correct results. It has the same effect in the D7 version (I rechecked) so it isn't caused by this patch. But it is worth mentioning. Maybe we need to convert the day and week days/numbers. But that needs to go into D7 first.

Besides of that I couldn't find any problems... No warnings, no errors, ... (and I test a lot) I suggest pushing this, at least we have a solid bas to work on.

tim.plunkett’s picture

Assigned: aspilicious » Unassigned
Status: Needs review » Fixed

Awesome, so nice to have this done. Now to backport the gcal stuff...
Oh, and open another issue for that date bug, I guess.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

SocialNicheGuru’s picture

this didn't make it into the 1.4 version but is needed there also.

aspilicious’s picture

Srry but this will never be integrated into the 1.4 version. You will need to upgrade if you would like all of this.