Changes for presentation logic of views summary

Please supply a proper patch.

Mojiro, I just downloaded your updated module but there do not appear to be any changes between it and the orginial 7.x-1.x-dev version of this module except for some added CSS modules.

If you will upload the changes I will try to make a proper patch for it.

All the Best,
Steve

subscribing

Attaching patch for handling currencies.
Thanks

The patch doesn't conform to Drupal coding standards:
- Indentation should always be two spaces.
- Comments should start with ”// ” (note the space).
- Currency code is unnecessarily loaded once for each amount, it should only be loaded once just before rendering.
- The function call doesn't need the argument names.

Additionally it seems to require Drupal Commerce, unlike the module itself.

Since the other fixes are no longer available, this seems to me like a duplicate of #1517570: Allow the change of currency.

Sadly the uploaded patch is completely different to what mojiro posted and currently no issue in the issue queue truly supports the changes he made. Does anyone have a copy of the file that he hosted? Sadly it is no longer available. Thanks in advance!

Attached is the "updated" module that I downloaded from mojiro.

Hope you find it useful.

Good Luck!

Thanks, @wickwood!

I fixed the issue title to reflect the changes made in the version uploaded by @wickwood in #10 and apparently originally uploaded by @mojiro. Additionally made a patch of the (relevant) changes, a few of the changes were coding style changes that didn't seem correct to me.

I didn't test the patch, but perhaps someone will find it useful. The patch is against 7.x-1.x current HEAD.

I was thinking about moving this to Needs review, as it applies and looks okay, but since I didn't test it I won't. If someone tests this and sees it working properly, it can be moved to Needs review and from there on to RTBC.

You guys are awesome. The fact that you still had the the file @wickwood is amazing. Will test this first thing Monday morning. You guys made a project's first presentation, that much better. Thank you.

After initial testing, I have found a few problems.

I was not able to apply the patch via "git apply -v". The following was shown when attempting to patch:

The following warning was displayed on pages where Views Summary was in-use:

Warning: implode(): Invalid arguments passed in include() (line 48 of /var/aegir/platforms/panopoly-7.x-1.43/sites/SITE/modules/views_summarize/views-summarize-views-tablesummarized.tpl.php).

I will add more, as I test further.

Which version were you applying the patch to? The patch is made against 7.x-1.x HEAD, so that may well differ from any release version.

@ZeiP: I downloaded it via git:

EDIT

Nevermind, it appears it was on my end after all. I may have downloaded the dev version on the project page actually.

Awesome. So now the aforementioned warning is the only problem so far.

Ok, one more issue found. Even though in the field settings (content type - manage fields) a suffix of "€" was added, the "€" suffix is not shown in the total.

However, using a comma as a decimal point, and using a decimal point as the thousands indicator works perfectly.

Also, the following notices are shown in watchdog.

Notice: Undefined index: in include() (line 50 of /var/aegir/platforms/panopoly-7.x-1.43/sites/SITE/modules/views_summarize/views-summarize-views-tablesummarized.tpl.php).
Notice: Undefined variable: count in include() (line 50 of /var/aegir/platforms/panopoly-7.x-1.43/sites/SITE/modules/views_summarize/views-summarize-views-tablesummarized.tpl.php).

Ok, it appears the suffix and prefix are instead new columns within the format settings of the view mode. Adding the suffix there worked perfectly and in turn was displayed in the total.

So after all of my testing, the only problems that I can concretely say exist is the aforementioned warning and the watchdog notices.

Edit:

Other than the small problems (notice and warnings), I think the only potential issue with the patch being pushed to dev, is that it'll break existing views configurations, since the currency option was removed (and instead merged with the standard "total" option, thanks to the suffix/prefix functionality). It'll probably need "something or other" in views_summary.install that'll update the existing configuration to a new one on update.

Also, would it make more sense to pull the prefix and the suffix from the field itself, like views core does by default? Or does the extra prefix/suffix settings make more sense in this case?

I'm going to work on adding the patch in #11 into the 7.x-1.x branch.
However, I agree with @NWOM in #18 that the prefix and the suffix settings should be in the field configuration along with the thousand and decimal markers.
Also, there is a bug (?) in that the Count summary displays as a decimal, instead of an integer, when the thousand and decimal markers are set, so I need to see what I can do to change that without changing other summaries that need to be decimals.

Okay, it turns out that putting those settings in the field settings is not a good idea after all.
I still think all the settings need to be together and applied to the individual summaries, rather than globally, so I need to figure something out. Placing all the fields in the primary settings table makes that table rather wide, but that might be the best place.

I'll be putting this in the new 7.x-2.x branch, so as not to disrupt people who are fine with what has been working for them.

I've been spending quite a bit of time, off and on, over the past few weeks. I had to make quite a few changes to fix some problems with the patch, but I think I finally have it where it needs be.

Here is what I changed:

1. I moved the thousand separator, decimal separator, and precision settings into the Style options table, along with the summarize, prefix, and suffix settings. That makes that table pretty wide, but it allowed me to overcome a few problems with putting them elsewhere.

There are two other places I could have used:

A) The first place was loose in the Style options form, like the "Display the summary row only" setting (where they were placed in the patch). Those are effectively global settings, so all summaries end up with the same settings regardless of their field type. So for example, setting a decimal separator and precision would mean that count summaries would always display the decimal separator followed by one or more zeros.

B) The second place was using the field settings themselves, i.e. there would be no specific settings for the summaries. The problem with that is that the summary could not be configured differently than the fields. That might not be a big deal for most things, but when averaging integers for example, it means the fractional part won't get displayed.

2. I had to modify several of the theme functions, because not all of the code completely or correctly accounted for the potential differences between the field settings and the summary settings (even before I made my changes). I tested this with many different fields and field configuration combinations. It is possible to have fields with one configuration of the thousand separator, decimal separator, and precision and the summary to have a different configuration, and the values will get determined correctly.

3. I updated the template, because the changes as submitted did not work when using "Display the summary row only" and produced error notices. I'm also not so sure about adding those classes there, but I left them and we'll see if anyone complains about their styling being messed up.

4. I kept the .css file, but I commented out the line in the .info file. I think anyone already using this has probably already styled their summaries and would likely not appreciate their styling being overridden (if that were to happen). I'll try to remember to add a note about that when this makes it into a tagged release.

I also created an update hook for people upgrading from a previous version.