GNOME Bugzilla – Bug 352287
share weather locations with gweather panel applet
Last modified: 2010-04-01 17:07:16 UTC
...which are in gnome-applets/gweather/Locations.xml.in this would fix bug 351563 and would make the weather applet finally useful. currently it isn't.
Yes, thats a good approach. Probably we can do it for 2.9.
What about to wait until the libgweather is done? (or "at least" usable for us)
milan: this is the plan for libgweather: http://mail.gnome.org/archives/desktop-devel-list/2008-January/msg00064.html so you could start if you want to. or you could even become the guy to propose an API architecture for libgweather. remember, this is 2008. the fun is over. ;-)
Bumping version to a stable release.
libgweather IS done now. go, go! ;-)
If someone takes this on, please update http://www.go-evolution.org/Evo2.24 by adding an item to the task list with your name. It's already listed there as a proposal.
is it wise to do it in such early development stage? :) #ifndef GWEATHER_I_KNOW_THIS_IS_UNSTABLE #error "libgweather should only be used if you understand that it's subject to change, and is not supported as a fixed API/ABI or as part of the platform" #endif
I think that just means we may have to occasionally adapt to ABI/API changes in libgweather during an unstable development cycle. Stable releases should not be a problem. D-BUS did something like that for a long time before finally reaching 1.0.
so - anybody working on this?
Created attachment 112613 [details] eds part (under construction)
Created attachment 112614 [details] evo part (under construction)
Created attachment 112615 [details] raw data Unfortunately, the thing libgweather returns is suitable only for gweather applet, even everything is stored in data structures, then return values are strings, which is kinda unusable for parsing. Also the forecast information, if available is in natural language, which is also kinda unusable for forecast events in Evolution. The changes above just demonstrates the possibility how to do this, but the concept of the change in eds can be different (like dropping ccf sources and adapt the parent class to our needs). The change in evo/calendar is only to show nice system weather icons, without that "test" change, the icons looks horrible. Nevertheless, there are required some changes in libgweather to let it work for us with minimal effort.
Created attachment 123186 [details] [review] proposed eds patch for evolution-data-server; The patch for libgweather from bug #538787 is required to have this running. One question came on my mind: is libgweather portable on Win32? I know Noell guys did quite much effort to have Evo running on Win32, and even I do not know if this backaend is included or not, then I made the libgweather as a requirement for eds and evo, thus if it's not portable, then quite bad luck and other approach should be chosen (like conditional compiling with old code if libgweather not available).
Created attachment 123187 [details] [review] proposed evo patch for evolution; Necessary changes in Evolution; notice other icon drawings in top item in calendar's day view. Without that those full color bitmaps are drawn in a very ugly way. Also, I didn't add any requirement to configure.in here, not sure if required or not, when using right eds.
<andre> vuntz, is libgweather portable on Win32? <andre> (cf bug 352287) <vuntz> andre: absolutely no idea <andre> k <vuntz> andre: I guess it could work <vuntz> with some work, though
(In reply to comment #13) > One question came on my mind: is libgweather portable on Win32? No idea. I guess it can work there -- probably just a few changes. We simply need someone who knows this kind of stuff :-)
(In reply to comment #16) > No idea. I guess it can work there -- probably just a few changes. We simply > need someone who knows this kind of stuff :-) Sure, no problem, I got similar response from Dan, but I wanted to write the question somewhere. Let's see whether Bharath will be able to do that or not :)
libgweather seems to be portable as such, I guess the only change needed to make it behave sensibly is to replace the hardcoded configure-time ZONEINFO_DIR with a run-time determination of it based on the location of the libgweather DLL, in the same way as done all over the place in the stack as ported to Windows already. (Ditto for GNOMELOCALEDIR.) I don't know if using zoneinfo data is really nice on Windows, though. Would it make more sense to instead just use the data as provided by the native Win32 timezone information?
Created attachment 123310 [details] [review] Suggested patch to libgweather This patch adds Win32 love to libgweather. Seems to work, at least test_locations worked and used the correct l10n for my locale. Presumably it would be a good idea to use the regex API in GLib instead of relying on a regex API in the C library or a separate libregex (as currently necessary on Win32).
Hmm, so this bug are about making evolution use libgweather for the *weather* stuff, right? Is there some other bug already open for issues related to what timezone API evolution could/should use on Windows? It's a bit silly to distribute 379 files of zoneinfo (from libical in e-d-s) or 77 MB of localised verbose XML (from libgweather) with Evolution when the Win32 API contains functions to get most of that information already. Unfortunately I am not entirely sure it is trivial to get all the required information from the Win32 API, though. The Win32 timezone API doesn't tell the geographic coordinates of some representative city that uses the timezone, for instance. And the GEO_TIMEZONES enumeration of the GetGeoInfo() API is "not implemented", says MSDN. But maybe I just need to browse more of the documentation to find out a way to connect GEOIDs with timezones.
Hmm, I was too optimistic about the Win32 API. Apparently there isn't any way to enumerate the time zones known by the system and get information for an arbitrary time zone. (Although the system surely has that information somewhere.)
Comment on attachment 123310 [details] [review] Suggested patch to libgweather Maybe it makes sense to move most of the stuff that is inside "#idef G_OS_WIN32" in a gweather-win32.h file and then include this file where it makes sense?
you mean the localtime_r definition? that's the only repeated stuff afaics.
This is awesome. We should get this in for 2.25.2 I'll try to give it some test before that.
Created attachment 123529 [details] Screen shot Cool, Milan, I applied the patches and it worked nicely. Before that i had built libgweather. Minor issues ============= -> Changes come into effect only after evolution restart. Like if i change Celsius to Fahrenheit, it will reflect only after restart. -> Africa continent has its bullets expanded. Every time i need to collapse them. -> I don't know what is the icon with New Delhi. In other places it is Day, cloud etc Enhancement ============ -> Looking for a particular place is not easy as we have in clock applet. General ======= I have attached the screen shot of calendar. I feel weather details should follow place name. Currently we have place name after weather info. And if we can make place name bold, it will be good. My thoughts, you can disagree :-)
We should be able to close bug 303308 , bug 345021 , bug 461677 after these patches will get committed.
(In reply to comment #25) > Minor issues > ============= > > -> Changes come into effect only after evolution restart. Like if i change > Celsius to Fahrenheit, it will reflect only after restart. I'm sorry, I cannot do anything here, see comment in source_changed in the http://svn.gnome.org/viewvc/evolution-data-server/trunk/calendar/backends/weather/e-cal-backend-weather.c?view=markup > -> Africa continent has its bullets expanded. Every time i need to collapse > them. I changed autoexpand in Evolution, but this one is inside libgweather/GtkTree, I can reproduce it even in weather applet. > -> I don't know what is the icon with New Delhi. In other places it is Day, > cloud etc I guess it's a fog, isn't it? You can see the detailed info when opening the whole day event. Order of the text in the summary changed. I also changed the way how it shows temperatures, before it showed "temp/dew" in the day and "temp_min/temp_max" in a forecast, which was a bit misleading. Thus changing the day to "temp_min/temp_max" if available, otherwise just "temp" is shown. > Enhancement > ============ > > -> Looking for a particular place is not easy as we have in clock applet. Yup, could be an enhancement bug, for the "find" feature which has the gweather applet. (it crashes to me, but no big deal). > General > ======= > > I have attached the screen shot of calendar. I feel weather details should > follow place name. Currently we have place name after weather info. > And if we can make place name bold, it will be good. I'm sorry, we cannot change bolding in the summary text, unfortunately, but changing order would work, I hope. > My thoughts, you can disagree :-) No no, I agree, thanks for them.
Created attachment 123729 [details] [review] proposed eds patch ][ for evolution-data-server; Applied changes mentioned by Akhil.
Created attachment 123730 [details] [review] proposed evo patch ][ for evolution; no autoexpand on evo's side when selecting the node. I added bug #562839 for the issue with expanding the first node.
(In reply to comment #25) > Created an attachment (id=123529) [edit] > Screen shot Is there a reason why these icons are different from those we have in the gnome 2.22 clock-applet that combines clock and weather?
(In reply to comment #30) > Is there a reason why these icons are different from those we have in the gnome > 2.22 clock-applet that combines clock and weather? evo has its own icons for the categories. Not so big deal to improve it, in other bug :)
I've committed something based on Tor's work to make libgweather work on windows. Of course, I'm pretty sure I broke stuff, so testing would be welcome ;-)
Milan, commit to trunk. Any other issues, we can take through bugs.
eds part committed to trunk. Committed revision 9793. evo part committed to trunk. Committed revision 36827. I changed libgweather version requirement to 2.25.3 in the eds part, it's otherwise same. Let's solve new/other issue(s) in new bug(s).
I'm reopening the bug in order to make the libgweather dependency optional. We should have a configure option for both e-d-s and evo (--with-libgweather) that defaults to 'yes' but if disabled will skip building the weather libedata-cal backend and Evolution plugin.
Have we not decided that we will be depending on the stable branch APIs for our gnome dependencies ?
Created attachment 124147 [details] [review] Build weather cal backend optionally Here's a patch to build the weather calendar backend optionally. Enabled by default, use --disable-weather to disable it - as always ;-) Something similar is needed for evolution too.
Created attachment 124153 [details] [review] Build weather cal plugin only if libgweather was found autotools are fun! I didn't expect it to be this simple ;-)
please don't make your autofoo autodetect stuff only and allow stuff to either be disabled, enabled or autodetected. Please see http://www.gentoo.org/proj/en/qa/automagic.xml for why this is important for downstream.
Agreed. The Evolution side should be explicit as well. Couple other things: - Please list the mimimum required gweather version at the top of the file alongside the others, so they're all in one place. - If the weather calendar is enabled on the Evolution side, consider testing for libebackendweather.so if not found, abort with a helpful message. I still need to test the e-d-s patch, but the approach looks correct.
(In reply to comment #36) > Have we not decided that we will be depending on the stable branch APIs for our > gnome dependencies ? We have. I guess this is a special case, though, since gweather is only a desktop library (not a platform library like GTK+) and they modified their API explicitly for us. But that's why I want the dependency optional, so I can still build Evolution 2.25 on GNOME 2.24.
(In reply to comment #40) > Agreed. The Evolution side should be explicit as well. I did try that route. More below. > - If the weather calendar is enabled on the Evolution side, consider testing > for libebackendweather.so if not found, abort with a helpful message. This is installed not in the usual prefix. libebackendweather.so is stashed away in {_libdir}/evolution-data-server-1.2/extensions/ . As such, AC_CHECK_LIB() can't be used (please correct me if I'm wrong here). A manual 'test' for the 'existence' of the .so needs to be carried out. Such a 'test' would have little meaning. I still agree that we should remove the automagic in evolution and make the dependency explicit. Only scenario where we need to worry about: e-d-s built with --disable-weather && evolution built with --enable-weather ^^ This I think would not be as catastrophic as it might appear. Evolution would compile with the plugin enabled. Only, no weather cal factories would be available when a user tries to create a weather calendar. Wouldn't crash, but wouldn't do anything else either :-)
Yeah, getting AC_CHECK_LIB() to work for that case might require some crazy LD_LIBRARY_PATH hack before calling the macro. Not sure. I think your proposal sounds fine as is.
Created attachment 124240 [details] [review] Build weather cal backend optionally better patch, since we are using an optional library, using --with seemed better than --enable
Created attachment 124241 [details] [review] Build weather cal plugin optionally okay.. specify explicit dependency on gweather and provide an option to skip building the plugin
Minor nitpick about the help strings. Write them in positive form: (EDS) --with-weather Build the weather calendar backend (Evo) --with-weather Build the weather calendar setup plugin Also a good idea to indicate the default: [Build the weather calendar backend [default=yes]] Apart from that, looks great! Approved.
(In reply to comment #46) > Minor nitpick about the help strings. Write them in positive form: > > (EDS) --with-weather Build the weather calendar backend > (Evo) --with-weather Build the weather calendar setup plugin I tried to follow the template at http://www.gentoo.org/proj/en/qa/automagic.xml#doc_chap2 :-) Anyway, I've changed it. > Also a good idea to indicate the default: > > [Build the weather calendar backend [default=yes]] Sure.. done that! > Apart from that, looks great! Approved. e-d-s patch committed to SVN trunk in r9821 http://svn.gnome.org/viewvc/evolution-data-server?view=revision&revision=9821 evolution patch committed to SVN trunk in r36860 http://svn.gnome.org/viewvc/evolution?view=revision&revision=36860
*** Bug 303308 has been marked as a duplicate of this bug. ***
*** Bug 345021 has been marked as a duplicate of this bug. ***
*** Bug 461677 has been marked as a duplicate of this bug. ***
The weather locations are still in the po files with the given advice at top: #. * These are the timezone names from the Olson timezone data. #. * We only place them here so gettext picks them up for translation. #. * Don't include in any C files. It's the proper way to leave the translations there? If I'm correct translators will have to translate the strings twice as always. So which is the benefit of the patches applied here? I just don't get it.
Milan, can you answer the last comment?
Oops, I'm sorry, I overlooked/forgot this. That Locations.xml.in is obsolete now, it seems, I forgot to get rid of it. Somehow, I cannot find the mentioned text above in sources, where does it come from? (That with preceding "#. *")
In addressbook/gui/contact-editor/e-contact-editor-address.c there are country names In calendar/zones.h there are the timezones. Cheers,
(In reply to comment #54) > In addressbook/gui/contact-editor/e-contact-editor-address.c there are country > names > > In calendar/zones.h there are the timezones. Thanks for the pointers. These have nothing to do with weather, though, but more interestingly, I didn't find a place where is used e-contact-editor-address.c, it's obsolete from my point of view, but maybe I searched incorrectly. I've no idea about that zones.h, it would take quite much time to find where exactly are these used in calendar.
(In reply to comment #55) > addressbook/gui/contact-editor/e-contact-editor-address.c what about removing and compiling? :) > I've no idea about that zones.h, it would take quite much time to find where > exactly are these used in calendar. evolution's calendar/zones.h and libical-0.43's zoneinfo/zones.h look completely the same to me.
(In reply to comment #55) > addressbook/gui/contact-editor/e-contact-editor-address.c > I didn't find a place where is used e-contact-editor-address.c, it's > obsolete from my point of view, but maybe I searched incorrectly. $:andre\> grep -r e-contact-editor-address.h . ./addressbook/gui/contact-editor/e-contact-editor.c: #include "e-contact-editor-address.h" > I've no idea about that zones.h, it would take quite much time to find where > exactly are these used in calendar. evolution's calendar/zones.h and libical-0.43's zoneinfo/zones.h look completely the same to me.
(In reply to comment #57) > $:andre\> grep -r e-contact-editor-address.h . > ./addressbook/gui/contact-editor/e-contact-editor.c: > #include "e-contact-editor-address.h" I know, it's included, but I didn't find usage from the header there, nor in the UI. Though it doesn't necessary mean it isn't used. (In reply to comment #56) > what about removing and compiling? :) Yes, that's one option. But it depends, whether it'll be used any time later or not, even I believe we both know the answer :)
Gil, can you file a separate ticket about comment 54 please?
(In reply to comment #59) > Gil, can you file a separate ticket about comment 54 please? > Done: #582922 Seems the country names are gone, so only the timezones are left.
*** Bug 272056 has been marked as a duplicate of this bug. ***