GNOME Bugzilla – Bug 564958
gweather doesn't show phase of the moon
Last modified: 2009-12-09 14:19:25 UTC
+++ This bug was initially created as a clone of Bug #170277 +++ Distribution/Version: Ubuntu Hoary "The gweather applet is currently showing a full moon when the moon is actually 17% full." Bug 170277 includes a proposed patch and icons.
Created attachment 125104 [details] [review] Changes to library files Assumes that weather_info_get_icon_name() should be limited to names in the default icon namespace. The caller determines if the name needs to be further adjusted.
Created attachment 125105 [details] new source file
I'm guessing you did not actually mean to close this, since the patch is not yet accepted/committed.
Right: sorry.. I'm also open to suggestion on the API, esp. wrt whether the library should have a set of icons rather than expect each caller to provide their own, then duplicate the code that determines which phase of the moon ought to be displayed.
Created attachment 125277 [details] [review] Alternative changes to library files Alternative approach: the application specifies the number of moon icons that are included in the adjusted icon path. The test for northern vs. southern hemisphere (to determine if shadows advance right to left or vice versa) is pushed down into the library.
just curious: any chance of this (along with bug 170277 and bug 565572) getting into 2.25.4 or 2.25.5?
2.25.4: nearly no chance, since it's today. 2.25.5, hopefully :-)
OK; thanks.. I noticed that 2.25.5 includes an API freeze; this patch adds one so it's probably the last chance for it to make it into 2.26.
the API freeze only applies to the Platform modules; libgweather is in the Desktop (and is explicitly non-API-stable anyway).
Quickly looked at this -- apologies for taking so long. And big thanks for working on this! I think it's wrong to have the user of the library caring about the moon: so both approaches you've proposed are not really good. I mean: I don't see why the clock applet should look for the right icon or why it should look at how many moon icons there are. So, we should probably do something with some feature of icon themes: if weather-clear-night-10 is what we want but does not exist in the icon theme, then GTK+ will automatically fallback to weather-clear-night. So let's step back and determine how many icons would be good-enough and then use them. I'm fine with shipping the icons with libgweather, fwiw. Now, various comments on the patch itself: === - if (ok) + if (ok) { info->sunValid = info->valid && calc_sun (info); - + info->moonValid = info->valid && calc_moon (info); + } if (!--info->requests_pending) Please keep an empty line before the second if. === +weather_info_get_value_moonphase (WeatherInfo *info, gdouble *value) I think the type for value is wrong: we want WeatherMoonPhase to avoid any doubt. === + * The algoithm is described in section 47 of Duffett-Smith Typo: algorithm === -static void -ecl2equ (gdouble time, gdouble eclipLon, - gdouble *ra, gdouble *decl) +void ecl2equ (gdouble time, + gdouble eclipLon, gdouble eclipLat, + gdouble *ra, gdouble *decl) Make this: void ecl2equ (... (two lines) === In ecl2equ: you're actually changing the formulas used. Is there a reason for that? === Can you give us a short explanation of the changes in gstObsv? === What's the point of this change: -#define DEGREES_TO_RADIANS(deg) ((fmod (deg,360.) / 180.) * M_PI) +#define DEGREES_TO_RADIANS(deg) ((fmod ((deg),360.) / 180.) * M_PI) === +gdouble sunEclipLongitude (time_t t); gboolean calc_sun (WeatherInfo *info); Remove one tab before sunEclipLongitude === Now, for weather-moon.c: + please add a license header, like the one in other files + instead of using comments like "// IN: time of interest", please use some gtk-doc function header. See gweather_location_entry_new(), for example. + "static void moonPosition (time_t t," please put "static void" on its own line + generally speaking, use a consistent style: one space before an opening parenthesis, and no space after, for example. + remove the empty line that starts calc_moon() and put an empty line before "return TRUE;"
Thanks for the feedback -- I wasn't crazy about pushing "number of moons" out to the app either but didn't like having the library add to the icon search path. If I can take it as a given that the icon theme already includes the capability of offering the fallback it makes the coding a lot simpler: "clock" and "gweather" don't need to be changed at all. The changes to ecl2equ were needed for the moon calculations. When it was being called only by calc_sun then the part of the expression starting with "tan(eclipLat)" was zero so that part of the calculation wasn't needed. gstObsv: formula corrections. They were minor when calculating sun positions but threw off the moon calculations by a large amount. When you mention "shipping the icons": should I adjust the installation process to install the icons someplace was well or just include the files as a reference? I'm finishing up the changes and should have it ready tonight (US ET). I wrote a perl script to generate the shadows icons and will add that as well.
Created attachment 127232 [details] [review] libgweather changes based on comments
Created attachment 127233 [details] new source file
Created attachment 127234 [details] script to generate shadows on an svg moon icon
Icons can be found at http://users.rcn.com/fenwayfrank/gweather-icons.bz2
Any chance of this getting included into 2.25.90?
Created attachment 128730 [details] new source file Fixes some calculation errors; includes a function to return estimates on when each upcoming phase of moon occurs (#170277 comment 13)
Created attachment 128731 [details] [review] Improved calculations; new function for upcoming phase-of-moon times
Do I need to make any other change on this?
Created attachment 136686 [details] [review] Updated to git patch Reworked as a git patch. Add test_sun_moon program for seeing specific results
Hi.. It's been about nine months since I submitted this. Any chance of this being included in 2.29?
So, the answer here is basically "no one is really maintaining libgweather". I kept hoping I'd find more time to devote to it at some point, but it's just not going to happen, and Vincent is even more over-extended GNOME-wise than I am. Long term we need to find a maintainer. Short term I'm not sure what to do with this patch.
mmm... I may regret saying this but I'd be willing to give it a shot. Where would I start? I just found http://live.gnome.org/MaintainersCorner ; anything else I should know about?
(oops, just noticed you changed the GNOME Target field; that's reserved for use by the release team.) http://live.gnome.org/LibGWeather has some notes on possible future plans. In particular, the Roadmap link discusses the current problems. The original plan for fixing this is under the "Improving Locations.xml" link, but at this point, I've come to decide that that isn't really a good idea, and we should just abandon Locations.xml in favor of using geonames.org's XML-RPC API to resolve city names. Reading through existing libgweather (and weather-applet and intlclock) bugs in bugzilla would also be a good way to get an idea of what this would entail.
*** Bug 565572 has been marked as a duplicate of this bug. ***
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.