531285 - Use OpenStreetmap for maps (even allow the user to choose from list of map services)

Status: RESOLVED FIXED

(Thunderbird :: Address Book, enhancement)

Description

[Thilo Pfennig]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.9.1.5) Gecko/20091109 Ubuntu/9.10 (karmic) Firefox/3.5.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.1.5) Gecko/20091121 Lightning/1.0pre Thunderbird/3.0

Currently Thunderbird uses Google maps. I think it would be nicer to use the open OpenStreetmap project.

Reproducible: Always

Steps to Reproduce:
if you click on "display on map" ...
Actual Results:  
your are lead to Google Maps

Expected Results:  
Land at OpenStreetMap based project.

Maybe OSM is not yet at the point where it makes sense to link to it, but I wanted to create that feature request to track the progress to get there. What is missing to use it?

Comment 1

[Ludovic Hirlimann [:Usul]]

Where is that display on map button ?

Comment 3

[Pander]

Please change this to platform independent and version 31.2.0

Ludovic, you will see it when you click on a contact in your address book (CTRL+SHIFT+B) and that contact has an address.

An implementation and workaround can be made easily on information found at http://bob.jonkman.ca/blogs/2012/10/09/how-to-use-openstreetmap-with-thunderbirds-get-map-button/

After having Thunderbird use OSM by default, as a second step, in Preferences configuration can be added so users can choose to use OSM (which is the default), but also switch Google Maps, Bing or Yahoo!

Comment 4

[Pander]

Well, just use only OSM will also stay clear if this discussion http://tech.slashdot.org/story/14/11/19/2313217/firefox-signs-five-year-deal-with-yahoo-drops-google-as-default-search-engine

Comment 5

[Pander]

I added a bounty via https://www.bountysource.com/issues/3493550-use-openstreemap-for-maps For those who support this as well, please make a donation too.

Comment 6

[Matt]

Modifying "mail.addr_book.mapit_url.format" in the config editor to "http://nominatim.openstreetmap.org/search.php?q=@A1,@A2,@CI,@ST,@CO" apparently implements this.  

So, is this bug is for a user interface element to set the value of the pref.

Or

A policy decision to change the default.

Like Ludo, I did not even know there was a button until I stumbled across this bug, so I do not know ho many people would actually use it. 

@:MCONOLY does this bug fall into the realm of Ensemnble as a feature?

Comment 7

[:aceman]

I don't think this relates to Ensemble, we can fix it independently.
As long as this is a global setting and mostly needs to change the pref value from some list of options, this should be doable today. We just need some specification for the feature, e.g.:
1. Where to put the setting? Today when you click Addressbook->Options, it opens in Options->Composition->Addressing. That doesn't look like the right place, but at least there is free space :) In current nightlies, there is a "default search engine" menu in the General tab of Options.  But I doubt the map selection would be allowed anywhere in the main Options dialog, only critical options go there.

Another option would be to change the current "get map" button to be a menu (dropdown list), from which you could choose in which map to find the address. This would work per contact. The choice could be stored (and actually remove the need of having the preference in the Options dialog).

2. Do we need to have the map URLs localizable? E.g. would different languages want to override them and put theirs there? E.g. maps.google.cn?

3. Are there any more mapping services, that we could support? If they are usable simply by constructing the URL and query string.

4. Do we want the user to easily add new maps, at least via creating prefs in the config editor?

Comment 8

[Magnus Melin [:mkmelin]]

A dropdown button could be useful. 
I suppose there are plenty of map providers, not sure we want to add any of the commercial vendors. OpenStreetmap has the open advantage of course.

Comment 9

[:aceman]

Attachment: patch

So this patch implements most of the meat of the feature.
From my questions the patch answers them like this:

>1. Where to put the setting?
>Another option would be to change the current "get map" button to be a menu (dropdown >list), from which you could choose in which map to find the address. This would work per >contact. The choice could be stored (and actually remove the need of having the preference >in the Options dialog).
A dropdown button as the "Get Map" button is implemented. Choosing the map service from the dropdown automatically saves the pref so no need to clutter up Options dialog.

>2. Do we need to have the map URLs localizable? E.g. would different languages want to >override them and put theirs there? E.g. maps.google.cn?
I found the current "mail.addr_book.mapit_url.format" is defined in the file region.properties so that is localizable (or overridable by region). So I continued in that schema and all the map services are localizable.

>3. Are there any more mapping services, that we could support? If they are usable simply >by constructing the URL and query string.
The patch allows adding more services in the future. Or various regions can define varying number of services themselves.

>4. Do we want the user to easily add new maps, at least via creating prefs in the config >editor?
This is still supported by the patch. If the user modifies the pref "mail.addr_book.mapit_url.format" (which is probably described in various docs), the value is preserved and copied to a userdefined pref "mail.addr_book.mapit_url.N.format" where N is next free position starting at 100. There can be unlimited user prefs like that.

So how do you like that?

Josiah, please see if the design is fine for you. I have not modified any strings. Tell me if there needs to be some new strings to polish this.

Paenglab, the dropdown menu from the button uses type="radio". But when opened, there is no "bullet" to indicate which is the current service. There is the "checked=true" attribute, but it has no effect. Can you check if this is a theme bug (in toolkit) or something with my code?

Comment 10

[Richard Marti (:Paenglab)]

Attachment: Win-menu-button.png

On Windows 7 the menu-button is looking weird with this padding around the inner button. I think it should more look like the "Write" button in main toolbar. How does it look on Linux and OS X (Josiah)?

This can be fixed in followup bug.

(In reply to :aceman from comment #9)
> Created attachment 8578203 [details] [diff] [review]
> patch
> 
> Paenglab, the dropdown menu from the button uses type="radio". But when
> opened, there is no "bullet" to indicate which is the current service. There
> is the "checked=true" attribute, but it has no effect. Can you check if this
> is a theme bug (in toolkit) or something with my code?

I compared with a working radio menuitem. The difference is, the working one is using checked="true" instead of selected="true". Could you try this?

Comment 11

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #10)
> Created attachment 8578268 [details]
> Win-menu-button.png
> 
> On Windows 7 the menu-button is looking weird with this padding around the
> inner button. I think it should more look like the "Write" button in main
> toolbar. How does it look on Linux and OS X (Josiah)?
On linux it is different. Only "get map" has border and grey background. The arrow has no border and no similar background so the white pane background shows through.

> I compared with a working radio menuitem. The difference is, the working one
> is using checked="true" instead of selected="true". Could you try this?

Yes, thanks, that worked. I wonder from which doc page I got the "selected".

Comment 12

[:aceman]

Attachment: patch v1.1

Thanks, fixed the radio "bullet" display.

Comment 13

[Josiah Bruner [:jsbruner]]

Hrm. I'm not really a fan of this.

Problems:
A. It looks odd (the padding/margins are quite significant).
B. There are currently no UI elements in OS X (and Linux and Windows AFAIK) that use a similar approach.
C. I find the fact that changing the map type also launches the map annoying.
D. On the same note, I don't like that you can't change the default without launching the map.

Solution?:

Could we use a button/dropdown like we do in the mail-toolbar. Something like the Forward button? Specifically, I mean having an "Open Map" button, with a dropdown that allows you to select the map type if you want.

Comment 14

[:aceman]

(In reply to Josiah Bruner [:JosiahOne] (needinfo > CC) from comment #13)
> Could we use a button/dropdown like we do in the mail-toolbar. Something
> like the Forward button? Specifically, I mean having an "Open Map" button,
> with a dropdown that allows you to select the map type if you want.
A+B:
This IS implemented the same as the Forward button (on main toolbar), with the exception that it is a <button> instead of <toolbar> button. But the only change I did is add the "type=menu-button" attribute. If the styling is wrong, than that needs to be fixed in the theme.

C+D:
I just thought why would anybody change the default without wanting to see the map. So the saving of the default would be a transparent side-feature without bothering the user. If you actually want to make it apparent to the user that it is a setting then that surely can be done.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: menu-button.patch

aceman, with this patch the menu-buttons are looking better on OS X and Windows.
I've added the styles in messenger.css to work globally in TB. Naturally it would be better in toolkit, but as FX doesn't use this type of button it's unlikely it would be accepted.

Comment 16

[:aceman]

I can't test that. Do you have screenshots of the patch on Win/OS X?
I'll show the default appearance on Linux.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: OSX-menu-button.png

This is how the fixed button looks on OS X Mountain Lion (tested through DOMi). Yosemite looks similar.

Comment 18

[Richard Marti (:Paenglab)]

Attachment: Win7-menu-button.png

Fixed button on Win7.

Comment 19

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #16)
> I can't test that. Do you have screenshots of the patch on Win/OS X?
> I'll show the default appearance on Linux.

Yeah, Linux looks like the toolbarbuttons.

Comment 20

[:aceman]

Attachment: Linux-menu-button

Comment 21

[:aceman]

Yeah, what you posted now looks quite good, similar to Linux. 
On Linux, the arrow has no border when not focused. Only when mouse is over it, it gets border and grey background so looks like in your images. Maybe you can add that border too so all platforms look similar.

Comment 22

[Richard Marti (:Paenglab)]

With your theme this would be no problem. With the theme I'm using or with other themes this wouldn't be so easy because of the radii and gradients they are using.

I think we can leave them on Linux as they are because we are already using this buttons without border around the dropmarker for the header buttons (smart reply and forward) and it's not a surprisingly new behavior.

Comment 23

[:aceman]

I'll be fine with that.

Comment 24

[Ludovic Hirlimann [:Usul]]

Did we talk to OSM before impletenting this ? Which tile server are we going to hammer ?(if our users use this feature a lot)

Comment 25

[Pander]

I have asked people from at least the Dutch OSM community to have a look at this and possibly discuss this in an international setting. Hope to see their comments here.

Comment 26

[Maarten Deen]

There is a tile usage policy on http://wiki.openstreetmap.org/wiki/Tile_usage_policy
Basically, OSM asks to
- seek permission to use the tiles from openstreetmap.org or
- use a different source.

Comment 27

[:aceman]

We are not using tile.openstreetmap.org directly and mass downloading any tiles. We just use their publicly exposed webpage at nominatim.openstreetmap.org. They can apply any throttling, load balancing and restrictions there for any users, not only TB, at their own discretion to maintain the service.

But sure, I will ask them explicitly. Thanks for pointing out the possibility that it could be restricted.

Did we have such permission from Google till now?

Comment 28

[Matt]

OpenStreetmap also suggest using MapQuest Open.

Just a suggestions here,  but this might be a potential revenue stream however small.  Offer Google, Bing, Yahoo, AOL (mapquest) and like Mozilla does with search, let them bid for the default spot.

Comment 29

[Pander]

Regardless if OSM Foundation has the funding or not, offer it also to them.

Comment 30

[:aceman]

If the revenue is too small, they may not even bother answer. Anyway, can you guys file a separate bug for adding more services and flesh out their order.

Let's keep this bug for the technical implementation of the feature. When it is done, we can discuss new services in followup bug. Remember the feature currently allows localization of the URLs so any language can use their own maps. So we could only "sell" the en-US version.

I could even keep Google Maps as the default entry to keep the status quo if promoting OSM as the default is a problem in this initial patch.

Comment 31

[Pander]

Good to split off the "selling" of en-US in another issues.

As for the default, on my smartphone I am forced to accept conditions for using Google Maps and if I don't accept these, I can't use that website. Of course, this is for an application on a desktop or laptop, so that is not the case. However, I am still being asked whether I would like to share my location with a site such as Google Maps. Many users accept that without being completely aware of all the implications.

Hence, I would like to propose having OSM as the default to offer (at least non-US) users safer usage. Could someone check if Mozilla supports this proposal for default? Since we are here to offer people a choice and are in the field of open source--even though most users are familiar with Google Maps--guiding people to OSM which is more up to date and safer to use should be promoted IMHO.

Comment 32

[:aceman]

(In reply to Pander from comment #31)
> As for the default, on my smartphone I am forced to accept conditions for
> using Google Maps and if I don't accept these, I can't use that website. Of
> course, this is for an application on a desktop or laptop, so that is not
> the case.

I think this is exactly the case. TB will only open the map search URL (with prefilled query string) in your default browser so the site may present you conditions to accept. We do not aim to override that in any way. We are not embedding any maps directly into TB.

Comment 33

[Pander]

That is not what I mean. I mean that people are guided to proprietary map website that want to know a lot about you while we could direct users to an open map website that doesn't do tracking.

Comment 34

[:aceman]

Yes, I would definitely be for OSM as default. I just say I'd not like to delve into such discussions in this bug, that should be focused in the implementation allowing tho then discuss and add any other maps. So if anybody has any slightly serious reason why Google should stay the default FOR NOW I would obey it.

I now need to address Josiah's ideas and upload new patch.

Comment 35

[Josiah Bruner [:jsbruner]]

I like Paenglab's changes, and I'd be fine with the UI as long as choosing a map *does not* open the map. I find that disruptive and surprising.

As for defaults... I need to think about that some more.

One the one hand, I do think OSM is more on par with Mozilla's mission, and of course admire its qualities. However, I have noticed that OSM does not work nearly as well as Google maps. For example, OSM couldn't locate my own home address!

So it comes down to mission vs. function and I'm sort of leaning towards the latter, but let me think some more.

Comment 36

[Pander]

Please create an issue at OSM of your home address could not be located. Mostly this concerns administrative city borders and alternative names or subdivisions.

Comment 37

[Kent James (:rkent)]

This bug as written in comment 0 is specifically about actually making OSM the default (along with solving any technical hurdles to get there). I suggest though that we follow aceman's suggestions in comment 30 "Let's keep this bug for the technical implementation of the feature.", and make this current bug more about just getting the technical infrastructure in place that allows OSM as an option, and deferring to another place discussions about changing the default.

If that is the case, then the patch needs to be changed to keep Google as the default.

That being said, I think it is unlikely that maps are going to be a useful source of income, and the choice of default would be a tradeoff between usability and open-source philosophy, which may vary per locale.

Comment 38

[:aceman]

(In reply to Kent James (:rkent) from comment #37)
> This bug as written in comment 0 is specifically about actually making OSM
> the default (along with solving any technical hurdles to get there).
There were basically no hurdles to get there, just changing the value of the pref for en-US.
However, that would probably not pass as the quality of OSM in US may not be as good as Google maps in US.
As filed, the bug would probably not be actionable and could be wontfixed (or lay around open for next 10 years until the situation changes).

I can confirm the quality of OSM in Europe (where also the reporter is) easily surpasses Google maps in most relevant regions.

So I apologize a bit for morphing the bug into a coding bug to implement the possibility of the USER (not en-US TB devs) to choose from several services according to his opinion. Regardless if he keeps en-US or changes language.

> I
> suggest though that we follow aceman's suggestions in comment 30 "Let's keep
> this bug for the technical implementation of the feature.", and make this
> current bug more about just getting the technical infrastructure in place
> that allows OSM as an option, and deferring to another place discussions
> about changing the default.
I've opened bug 1144761 for it.

> If that is the case, then the patch needs to be changed to keep Google as
> the default.
Will do.

> That being said, I think it is unlikely that maps are going to be a useful
> source of income, and the choice of default would be a tradeoff between
> usability and open-source philosophy, which may vary per locale.
Yes, localisers can already override it and with the patch here can just extend their list to more than one alternative service.

Comment 39

[:aceman]

Attachment: patch v1.2

Comment 40

[:aceman]

Comment on attachment 8579639 [details] [diff] [review]
patch v1.2

Ian, I can try to apply this for SM too, if you look over the TB version whether it is OK and I can make the same for SM.

Comment 41

[:aceman]

(In reply to Ludovic Hirlimann [:Usul] from comment #24)
> Did we talk to OSM before impletenting this ? Which tile server are we going
> to hammer ?(if our users use this feature a lot)

I have got permission from OSM administrator to provide the link to their server. I have the email at my disposal. I can provide it to Mozilla if needed.

Comment 42

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #41)
> I have got permission from OSM administrator to provide the link to their
> server. I have the email at my disposal. I can provide it to Mozilla if
> needed.

What about attaching it to this bug?

Comment 43

[Josiah Bruner [:jsbruner]]

Comment on attachment 8579639 [details] [diff] [review]
patch v1.2

Review of attachment 8579639 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I'm happy with this. One issue though, I now have two OSM choices. OpenStreetMap and nominatim.openstreetmap.org. Probably just a slight previous-patch merging bug though.

Comment 44

[Josiah Bruner [:jsbruner]]

Attachment: Addition option?

Here's what it looks like. I doubt that's intentional.

Comment 45

[:aceman]

I changed the URL for openstreetmap slightly in the new patch. So if you tested the previous patch and had the old URL as the default, then the result is intentional. I said it in comment 9, if the user modifies the pref (the default one) as was the practice till now, his URL is added as a new item in the list. The result is there will be 1-5 URLs predefined from the localization file. And the user can add any number of custom URLs starting at position 100.

Comment 46

[:aceman]

(In reply to Josiah Bruner [:JosiahOne] (needinfo! - Won't respond to CC)) from comment #44)
> Here's what it looks like. I doubt that's intentional.
So yes, this result is intentional for a user that sets his own URLs. You just did it unintentionally by testing the patches :)

Comment 47

[Magnus Melin [:mkmelin]]

Comment on attachment 8579639 [details] [diff] [review]
patch v1.2

Review of attachment 8579639 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +561,2 @@
>  {
> +  let URL = null;

please just use lowercase "url"

@@ +567,5 @@
> +    } else {
> +      URL = Services.prefs.getComplexValue("mail.addr_book.mapit_url." + aIndex + ".format",
> +                                           nsILocString).data;
> +    }
> +  } catch (e) { }

this try-catch seems more of a trap than it would be useful. if something's wrong at least with the error you'd have a chance to figure it out

@@ +583,5 @@
> +{
> +  let index = 1;
> +  let itemFound = true;
> +  let defaultFound = false;
> +  const userIndex = 100;

k prefixed or ALL_CAPS

@@ +602,5 @@
> +      item.setAttribute("checked", "true");
> +    aMapButton.appendChild(item);
> +  }
> +
> +  // Generates a useful generic name by cutting out only the host address.

there's "new URL(url).hostname"

@@ +711,3 @@
>  {
> +
> +  let URL = CreateMapItURL(aMapButton);

let url =

::: mailnews/mailnews.js
@@ +128,5 @@
>  pref("mail.file_attach_binary",             false);
>  pref("mail.show_headers",                   1); // some
>  pref("mail.pane_config.dynamic",            0);
>  pref("mail.addr_book.mapit_url.format", "chrome://messenger-region/locale/region.properties");
> +pref("mail.addr_book.mapit_url.1.name", "chrome://messenger-region/locale/region.properties");

I'm not sure these should be in mailnews.js even if seamonkey want it too later, since the code using it is not shared.

Comment 48

[:aceman]

(In reply to Magnus Melin from comment #47)
> @@ +567,5 @@
> > +    } else {
> > +      URL = Services.prefs.getComplexValue("mail.addr_book.mapit_url." + aIndex + ".format",
> > +                                           nsILocString).data;
> > +    }
> > +  } catch (e) { }
> 
> this try-catch seems more of a trap than it would be useful. if something's
> wrong at least with the error you'd have a chance to figure it out

There is nothing wrong, I just expect that we hit an aIndex for which the pref does not exist. So it will throw and url gets returned as null so we detect the condition and stop iterating the prefs. Is there a better way to detect there is no pref?

> @@ +602,5 @@
> > +      item.setAttribute("checked", "true");
> > +    aMapButton.appendChild(item);
> > +  }
> > +
> > +  // Generates a useful generic name by cutting out only the host address.
> 
> there's "new URL(url).hostname"
Nice!

> ::: mailnews/mailnews.js
> @@ +128,5 @@
> >  pref("mail.file_attach_binary",             false);
> >  pref("mail.show_headers",                   1); // some
> >  pref("mail.pane_config.dynamic",            0);
> >  pref("mail.addr_book.mapit_url.format", "chrome://messenger-region/locale/region.properties");
> > +pref("mail.addr_book.mapit_url.1.name", "chrome://messenger-region/locale/region.properties");
> 
> I'm not sure these should be in mailnews.js even if seamonkey want it too
> later, since the code using it is not shared.

Code is not shared but it will probably do the same thing. mail.addr_book.mapit_url.format is in mailnews.js so why should the others not be? But yes, if SM would like to potentially do something else with the prefs, they need to be separated.

Comment 49

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #48)
> (In reply to Magnus Melin from comment #47)
> > @@ +567,5 @@
> > > +    } else {
> > > +      URL = Services.prefs.getComplexValue("mail.addr_book.mapit_url." + aIndex + ".format",
> > > +                                           nsILocString).data;
> > > +    }
> > > +  } catch (e) { }
> > 
> > this try-catch seems more of a trap than it would be useful. if something's
> > wrong at least with the error you'd have a chance to figure it out
> 
> There is nothing wrong, I just expect that we hit an aIndex for which the
> pref does not exist. So it will throw and url gets returned as null so we
> detect the condition and stop iterating the prefs. Is there a better way to
> detect there is no pref?

I guess not, but please only do the try-catch for that particular case, not for the default also. And report the error to the console if it occurs.

> Code is not shared but it will probably do the same thing.
> mail.addr_book.mapit_url.format is in mailnews.js so why should the others
> not be? But yes, if SM would like to potentially do something else with the
> prefs, they need to be separated.

I doubt they would do something else with it. What I meant is that there's no reason to keep it in mailnews.js if it's not shared code, that may or may not (theoretically) do the same thing. Once both apps use it you're then locked in not to change it in one but both if you for some reason want to change the behavior on one.

Comment 50

[:aceman]

(In reply to Magnus Melin from comment #49)
> > Code is not shared but it will probably do the same thing.
> > mail.addr_book.mapit_url.format is in mailnews.js so why should the others
> > not be? But yes, if SM would like to potentially do something else with the
> > prefs, they need to be separated.
>
> I doubt they would do something else with it. What I meant is that there's
> no reason to keep it in mailnews.js if it's not shared code, that may or may
> not (theoretically) do the same thing. Once both apps use it you're then
> locked in not to change it in one but both if you for some reason want to
> change the behavior on one.

So do you propose to also pull the existing pref of mail.addr_book.mapit_url.format out of mailnews.js ?

I'd hope we strive to merge code when possible, not create even more forks.

Comment 51

[Magnus Melin [:mkmelin]]

I don't think we need to move the existing pref, but would not be against it either. 

What we strive to do is create code that is easy to maintain and change as needed. Forks are not always a bad thing for that. But ok, I'll let you decide what you feel is appropriate in this case.

Comment 52

[Ian Neal]

Comment on attachment 8579639 [details] [diff] [review]
patch v1.2

>+++ b/mail/components/addrbook/content/abCardViewOverlay.js

>+const nsILocString = Components.interfaces.nsIPrefLocalizedString;
Nit: maybe nsIPrefLocalizedString as the const name?

>+++ b/mail/locales/en-US/chrome/messenger-region/region.properties
If touching region.properties should it be run past the productisation team?

>+++ b/mailnews/mailnews.js
>+pref("mail.addr_book.mapit_url.1.name", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.1.format", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.2.name", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.2.format", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.3.name", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.3.format", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.4.name", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.4.format", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.5.name", "chrome://messenger-region/locale/region.properties");
>+pref("mail.addr_book.mapit_url.5.format", "chrome://messenger-region/locale/region.properties");

To me it makes sense that all the mapit_url prefs are in the same file, but could the changes to region.properties be replicated in suite/locales/en-US/chrome/mailnews/region.properties?

Thanks.

Comment 53

[:aceman]

(In reply to Ian Neal from comment #52)
> >+++ b/mail/locales/en-US/chrome/messenger-region/region.properties
> If touching region.properties should it be run past the productisation team?
Who is that? Can you CC them here?

> To me it makes sense that all the mapit_url prefs are in the same file, but
> could the changes to region.properties be replicated in
> suite/locales/en-US/chrome/mailnews/region.properties?
Sure, I will copy the feature (code + region properties) to Seamonkey now.

I understood from Magnus that he proposes to copy the whole block into /mail and /suite/, including the default url (without number):
+++ b/mailnews/mailnews.js
 pref("mail.addr_book.mapit_url.format", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.1.name", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.1.format", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.2.name", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.2.format", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.3.name", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.3.format", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.4.name", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.4.format", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.5.name", "chrome://messenger-region/locale/region.properties");
+pref("mail.addr_book.mapit_url.5.format", "chrome://messenger-region/locale/region.properties");

Could we put this whole button and code into mailnews/addrbook/content/addrbookWidgets.xml to be in a shared place to prevent the code copies to drift apart in functionality and usage of the prefs? In that case we could keep the prefs in mailnews.js. Or does Seamonkey want to preserve the option to drift away with the code? As I am not sure why the AB is actually forked.

Comment 54

[Magnus Melin [:mkmelin]]

Personally I wouldn't bother moving it. XBL is such an odd bird in general, though of course it has it's upsides too...

Comment 55

[Ian Neal]

One of the reasons for AB forking is the chat component, but there is still a lot of common code between TB and SM forks. There is some unforked code sitting in mailnews/addrbook/content/

Comment 56

[:aceman]

Yes there is some unforked code in the mailnews files. But that still doesn't answer if I should put this code there ;) As it was a button it seemed to me the right place would be in the .xml bindings. But I could also just put the code in the shared files and put the copies of the buttons into the forked AB files.

Comment 57

[:aceman]

Attachment: patch v2

I have moved the code to a shared location in addressbookWidgets.xml so that the pref can also stay in shared mailnews.js.

The constructor of the binding isn't the greatest as the child menupopup should be possible to do in a <content> block of the binding. But I didn't find out how to do that, without breaking the underlying menu-button binding.

The patch should also contain everything needed for Seamonkey so you can properly test and review it.

Comment 58

[Ian Neal]

(In reply to :aceman from comment #57)
> Created attachment 8600660 [details] [diff] [review]
> patch v2
> 
> I have moved the code to a shared location in addressbookWidgets.xml so that
> the pref can also stay in shared mailnews.js.
> 
> The constructor of the binding isn't the greatest as the child menupopup
> should be possible to do in a <content> block of the binding. But I didn't
> find out how to do that, without breaking the underlying menu-button binding.
> 
Would this be because it is a class rather and a type and the <content> block from the existing type conflicts/overwrites the one you were trying to use for the map-button? What showed up when you used a DOM inspector on the map-button?

Comment 59

[:aceman]

(In reply to Ian Neal from comment #58)
> Would this be because it is a class rather and a type and the <content>
> block from the existing type conflicts/overwrites the one you were trying to
> use for the map-button? What showed up when you used a DOM inspector on the
> map-button?

I didn't even try DOM Inspector as I directly got an error in console:
XBL binding for <button type="menu-button"/> binding must contain an element with anonid="button".
It looked like my <content> was replacing all of the content from the original menu-button binding so there was even no <button> defined (the widget showed up empty). I tried to copy all the content to my binding but it still didn't work. But I do not want to do that in the final version as I do not change the appearance in any way, just attach the special oncommands and functions. I'd like to inherit everything possible from the standard menu-button.

Comment 60

[[e-mail]]

Comment on attachment 8600660 [details] [diff] [review]
patch v2

IanN suggested that I might want to do a drive-by review, so I've just looked at the code without actually trying it out.

>+          this.setAttribute("oncommand", "this._MapIt();");
This should be a <handler>.

>+          let popup = document.createElement("menupopup");
(I'm not sure whether you can, or should, automatically create this in content.)

>+          popup.setAttribute("onpopupshowing", "this.parentNode._ListMapServices();");
This could be a <handler>.

>+          popup.setAttribute("oncommand", "this.parentNode._ChooseMapService(event.target);event.stopPropagation();");
This could be a <handler> too... well, you would check the event target in your command handler.

>+          let mapItURLFormat = this._GetMapURLPref(0);
>+          if (!mapItURLFormat || !aAddrPrefix || !aCard) {
>+            this.setAttribute("collapsed", "true");
Nit: could use this.collapsed

>+            return;
>+          }
Don't you need to show the button again when you get a valid link?

>+          this.setAttribute("map_address1", aCard.getProperty(aAddrPrefix + "Address"));
>+          this.setAttribute("map_address2", aCard.getProperty(aAddrPrefix + "Address2"));
>+          this.setAttribute("map_city"    , aCard.getProperty(aAddrPrefix + "City"));
>+          this.setAttribute("map_state"   , aCard.getProperty(aAddrPrefix + "State"));
>+          this.setAttribute("map_zip"     , aCard.getProperty(aAddrPrefix + "ZipCode"));
>+          this.setAttribute("map_country" , aCard.getProperty(aAddrPrefix + "Country"));
Nit: Not necessary to use attributes, you could use properties, or just stash the card and prefix.

>+          if (!aIndex)
>+            aIndex = 0;
>+          if (aIndex == 0) {
Nit: could write this as if (!aIndex) {

>+          const kUserIndex = 100;
Why 100? Users will probably try to edit the existing 5 entries first anyway; you might as well start at 1 and stop when there's no pref type.

>+          let newUrl = aItem.getAttribute("url");
>+          defaultUrl.data = newUrl;
Nit: Could do this on one line.

>+mail.addr_book.mapit_url.2.format=http://nominatim.openstreetmap.org/search.php?q=@A1%2C@A2%2C@CI%2C@ST%2C@ZI%2C@CO&polygon=1
[Hmm, the Google url uses %20 and not %2C? Although, I think my preference would be for both, %2C%20...]

>+.map-button {
>+  -moz-binding: url("chrome://messenger/content/addressbook/addrbookWidgets.xml#map-button");
>+}
[Bah, this should be in chrome css, but the addressbook doesn't have any.]

Having finished reading the patch, I think you're trying to do too much at once in the binding. My suggestion would be to add the <menupopup> in the addressbook.xul document, bind the map picker XBL to the menupopup, and tweak the existing MapIt code not to cache the map URL.

Comment 61

[Ian Neal]

Comment on attachment 8600660 [details] [diff] [review]
patch v2

f+ for the moment, see how you progress with Neil's comments.

Comment 62

[:aceman]

(In reply to [e-mail] from comment #60)
> Comment on attachment 8600660 [details] [diff] [review]
> patch v2
> 
> IanN suggested that I might want to do a drive-by review, so I've just
> looked at the code without actually trying it out.
Thanks. I fixed most of the problems. For those that I did not change I provide the explanation now.

> >+          let popup = document.createElement("menupopup");
> (I'm not sure whether you can, or should, automatically create this in
> content.)
I can, as it does work. I didn't want to as it seams unclean, but I didn't find a way to do it via <content>. So should I just put an empty <menupopup> into the callers ?
 
> >+            return;
> >+          }
> Don't you need to show the button again when you get a valid link?
I think we did not update the data dynamically in the contact preview. So there was not a case to re-show the button. But OK, I implement it to make the binding more universal for other uses (as I write below).

> >+          this.setAttribute("map_address1", aCard.getProperty(aAddrPrefix + "Address"));
> >+          this.setAttribute("map_address2", aCard.getProperty(aAddrPrefix + "Address2"));
> >+          this.setAttribute("map_city"    , aCard.getProperty(aAddrPrefix + "City"));
> >+          this.setAttribute("map_state"   , aCard.getProperty(aAddrPrefix + "State"));
> >+          this.setAttribute("map_zip"     , aCard.getProperty(aAddrPrefix + "ZipCode"));
> >+          this.setAttribute("map_country" , aCard.getProperty(aAddrPrefix + "Country"));
> Nit: Not necessary to use attributes, you could use properties, or just
> stash the card and prefix.
Yes, I thought about that. But using plain attributes makes the binding/widget more universal so that it can also be used without a card. E.g. we could show a map of some address that does not have a backing card. E.g. an address in a received message's signature, if we could detect it. Or the calendar could store address of a meeting and we could look it up via this widget. That is another reason why I try to put this binding into a shared location and not hardcode it into AbCardViewOVerlay.
I have now added another initializing function to make this support without a backing card more explicit.

> >+          const kUserIndex = 100;
> Why 100? Users will probably try to edit the existing 5 entries first
> anyway; you might as well start at 1 and stop when there's no pref type.
Yes, they can edit the first 5, thus forking away from possible improvements to urls from the localizer. But I want to leave the option of the first 5 being localization dependent and being updated by the localizer in the future. So for that the user links are added at another higher position. The position 100+ is actually used automatically if users modify mail.addr_book.mapit_url.format as is documented in all the manuals. So the users do not need to think about this and just follow existing practice.

> >+mail.addr_book.mapit_url.2.format=http://nominatim.openstreetmap.org/search.php?q=@A1%2C@A2%2C@CI%2C@ST%2C@ZI%2C@CO&polygon=1
> [Hmm, the Google url uses %20 and not %2C? Although, I think my preference
> would be for both, %2C%20...]
I don't know what Google should use but I intentionally don't touch the current Google url. We have another bug for thinking about changing the current urls.

> Having finished reading the patch, I think you're trying to do too much at
> once in the binding. My suggestion would be to add the <menupopup> in the
> addressbook.xul document, bind the map picker XBL to the menupopup, and
> tweak the existing MapIt code not to cache the map URL.
Surely I want to do as much as possible in the shared place so that the widget can't be broken by incorrectly setting it up and code does not need to be duplicated at the callers.

Comment 63

[:aceman]

Attachment: patch v2.1

Added Neil's improvements as I described.

Comment 64

[:aceman]

Attachment: patch v2.2

Small copy problem :)

Comment 65

[[e-mail]]

(In reply to aceman from comment #62)
> (In reply to comment #60)
> > >+          let popup = document.createElement("menupopup");
> > (I'm not sure whether you can, or should, automatically create this in
> > content.)
> I can, as it does work. I didn't want to as it seams unclean, but I didn't
> find a way to do it via <content>. So should I just put an empty <menupopup>
> into the callers ?
I suspected that <content> probably wouldn't work. I have a slight preference for putting the <menupopup> into the XUL, or failing that, double-checking that it's not already there.

> > >+            return;
> > >+          }
> > Don't you need to show the button again when you get a valid link?
> I think we did not update the data dynamically in the contact preview.
Well, at the very last, we update it when you change the selected card...

> > >+          const kUserIndex = 100;
> > Why 100? Users will probably try to edit the existing 5 entries first
> > anyway; you might as well start at 1 and stop when there's no pref type.
> Yes, they can edit the first 5, thus forking away from possible improvements
> to urls from the localizer. But I want to leave the option of the first 5
> being localization dependent and being updated by the localizer in the
> future. So for that the user links are added at another higher position. The
> position 100+ is actually used automatically if users modify
> mail.addr_book.mapit_url.format as is documented in all the manuals. So the
> users do not need to think about this and just follow existing practice.
Sure, but that doesn't explain the hole between 5 and 100.

> > Having finished reading the patch, I think you're trying to do too much at
> > once in the binding. My suggestion would be to add the <menupopup> in the
> > addressbook.xul document, bind the map picker XBL to the menupopup, and
> > tweak the existing MapIt code not to cache the map URL.
> Surely I want to do as much as possible in the shared place so that the
> widget can't be broken by incorrectly setting it up and code does not need
> to be duplicated at the callers.

That depends on what you want the widget to do. As it happens, the functions to open URLs are different on Thunderbird and SeaMonkey, and it might not be optimal to hard-code it into the widget. (Worse, we're inconsistent on SeaMonkey; we probably should use openAsExternal rather than openTopWin.)

Comment 66

[[e-mail]]

Comment on attachment 8604817 [details] [diff] [review]
patch v2.2

>+          event.target.parentNode._ListMapServices();
In a <handler>, this == event.currentTarget, so you can just write this._ListMapServices(); etc. (Why the capital L?)

Comment 67

[:aceman]

(In reply to [e-mail] from comment #65)
> > > >+            return;
> > > >+          }
> > > Don't you need to show the button again when you get a valid link?
> > I think we did not update the data dynamically in the contact preview.
> Well, at the very last, we update it when you change the selected card...
And does that not work with my implementation?
In bug 1164170, there will be no card backing the address, so I do not want to tie the implementation to any card object.

> > > >+          const kUserIndex = 100;
> > > Why 100? Users will probably try to edit the existing 5 entries first
> > > anyway; you might as well start at 1 and stop when there's no pref type.
> > Yes, they can edit the first 5, thus forking away from possible improvements
> > to urls from the localizer. But I want to leave the option of the first 5
> > being localization dependent and being updated by the localizer in the
> > future. So for that the user links are added at another higher position. The
> > position 100+ is actually used automatically if users modify
> > mail.addr_book.mapit_url.format as is documented in all the manuals. So the
> > users do not need to think about this and just follow existing practice.
> Sure, but that doesn't explain the hole between 5 and 100.
The number of the "base" shipped urls can change. Is the hole too big? So which number would you propose for the user index?

> > > Having finished reading the patch, I think you're trying to do too much at
> > > once in the binding. My suggestion would be to add the <menupopup> in the
> > > addressbook.xul document, bind the map picker XBL to the menupopup, and
> > > tweak the existing MapIt code not to cache the map URL.
> > Surely I want to do as much as possible in the shared place so that the
> > widget can't be broken by incorrectly setting it up and code does not need
> > to be duplicated at the callers.
> 
> That depends on what you want the widget to do. As it happens, the functions
> to open URLs are different on Thunderbird and SeaMonkey, and it might not be
> optimal to hard-code it into the widget. (Worse, we're inconsistent on
> SeaMonkey; we probably should use openAsExternal rather than openTopWin.)
Yeah, I see, I haven't yet removed the SM MapIt* functions. I'll see how to pass the function to open the URL from the caller into the widget.

Comment 68

[:aceman]

Attachment: patch v3

Would you like this better? This almost implements your proposal. The binding is on the menupopup and the button must provide its own oncommand to call the right function to open the url.

Comment 69

[[e-mail]]

Comment on attachment 8609902 [details] [diff] [review]
patch v3

>+      <constructor>
>+        <![CDATA[
>+        ]]>
>+      </constructor>
[Don't need this]

>+          this.collapsed = (!mapItURLFormat || !aAddrPrefix || !aCard);
>+          if (this.collapsed)
>+            return;
[Not sure this is correct.]

Comment 70

[[e-mail]]

(In reply to aceman from comment #67)
> (In reply to comment #65)
> > > > Don't you need to show the button again when you get a valid link?
> > > I think we did not update the data dynamically in the contact preview.
> > Well, at the very last, we update it when you change the selected card...
> And does that not work with my implementation?
I'd just found it a bit confusing; the card view code shows the button if it thinks there's an address but the button then hides itself again if there's no link.

> The number of the "base" shipped urls can change.
So you're allowing for 5 for now, but up to 100 in future?

Comment 71

[:aceman]

(In reply to [e-mail] from comment #70)
> (In reply to aceman from comment #67)
> > (In reply to comment #65)
> > > > > Don't you need to show the button again when you get a valid link?
> > > > I think we did not update the data dynamically in the contact preview.
> > > Well, at the very last, we update it when you change the selected card...
> > And does that not work with my implementation?
> I'd just found it a bit confusing; the card view code shows the button if it
> thinks there's an address but the button then hides itself again if there's
> no link.
Yes. I can look at that. It is possible that that doesn't make sense now when the binding is on the menulist.

> > The number of the "base" shipped urls can change.
> So you're allowing for 5 for now, but up to 100 in future?
Exactly. It depends on how many "base" urls we pull in in mailnews/mailnews.js . Of course I do not expect to have 100 ever in the future, but it looked like a nice round number :) Feel free to propose any other constant.

> >+          this.collapsed = (!mapItURLFormat || !aAddrPrefix || !aCard);
> >+          if (this.collapsed)
> >+            return;
> [Not sure this is correct.]
If the binding stays on the menulist, this is NOT correct.

Comment 72

[:aceman]

(In reply to :aceman from comment #71)
> Yes. I can look at that. It is possible that that doesn't make sense now
> when the binding is on the menulist.
I mean menupopup.

> > >+          this.collapsed = (!mapItURLFormat || !aAddrPrefix || !aCard);
> > >+          if (this.collapsed)
> > >+            return;
> > [Not sure this is correct.]
> If the binding stays on the menulist, this is NOT correct.
I mean menupopup.

So should I go on cleaning up this version? Do you like it more on the menupopup than the button?

Comment 73

[:aceman]

Attachment: patch v3.1

I cleaned up the state of the widget now. The binding can decide to collapse the button if there is no map url. If there is one, but no address was set (yet), it disables the button. If a map url suddenly becomes available (e.g. user going into advanced prefs) the button does not suddenly appear.
The separation of collapsed and disabled state was done for potential future use, e.g. in the planned calendar use, where the button could be always visible (if map url exists) but may enable/disable itself depending on whether user typed anything into the event's Location field. This dynamic state is not needed in the AB card display pane (where data never changes), but can be use full elsewhere.

Comment 74

[[e-mail]]

(In reply to aceman from comment #72)
> Do you like it more on the menupopup than the button?

Yes, thanks.

(In reply to aceman from comment #73)
> The binding can decide to collapse the button if there is no map url.

I think I'd prefer it if it didn't. It's already disabled, which should be enough. (In fact, you could then argue the case that it will help you identify the "no map url" case.)

Comment 75

[:aceman]

(In reply to [e-mail] from comment #74)
> (In reply to aceman from comment #73)
> > The binding can decide to collapse the button if there is no map url.
> 
> I think I'd prefer it if it didn't. It's already disabled, which should be
> enough. (In fact, you could then argue the case that it will help you
> identify the "no map url" case.)

I assumed hiding the widget is intentional feature for users that are able to go into about:config and clear the URL. I just preserved the feature. If it is no longer needed to do so, we can simplify it.

Comment 76

[:aceman]

Attachment: patch v3.2

Ok, I have pulled out hiding of the button back to the AB code. So if AB wants to hide the button if no URL is defined, the functionality is preserved. Other users of the widget will not be forced to that behaviour.

Comment 77

[[e-mail]]

Comment on attachment 8660427 [details] [diff] [review]
patch v3.2

>+  let mapURLList = data.cvHomeMapIt.querySelector("menupopup");
You still have .firstChild in the XUL, so these are inconsistent.
Also if you don't like .firstChild then use .menupopup instead.

r=me with those fixed.

Comment 78

[:aceman]

Attachment: patch v3.3

I'd love to use .menupopup if it existed on <button>. So I've made it consistent with .firstChild.

Comment 79

[aleth [:aleth]]

menu-button.patch fails to apply.

Comment 80

[Richard Marti (:Paenglab)]

Attachment: menu-button.patch

The patch is only needed for OS X. Windows is already fixed.

Comment 81

[:aceman]

Comment on attachment 8662556 [details] [diff] [review]
menu-button.patch

Carrying over ui-r+ from Josiah from the previous version. This one only cuts out the unneded Windows part.

Comment 82

[Philip Chee]

Comment on attachment 8661950 [details] [diff] [review]
patch v3.3

> +.map-list {
> +  -moz-binding: url("chrome://messenger/content/addressbook/addrbookWidgets.xml#map-list");

Please move the two suite bindings to suite/mailnews/messenger.css somewhere near .addrbooksPopup
Thank you.

> +++ b/mail/themes/shared/mail/addressbook.css
....
> +.map-list {
> +  -moz-binding: url("chrome://messenger/content/addressbook/addrbookWidgets.xml#map-list");
> +}

Added bonus:

Move the above to mail/base/content/messenger.css somewhere near .addrbooksPopup

(messenger/skin/addressbook/addressbook.css loads messenger/skin/messenger.css which loads messenger/content/messenger.css)

Third party theme authors should not have to deal with this sort of bindings.

Comment 83

[:aceman]

Attachment: patch v3.4

I didn't notice there are "internal" css files in the code directories. Of course, that is a better place, thanks.

Comment 84

[Philip Chee]

Comment on attachment 8663117 [details] [diff] [review]
patch v3.4

a=me for SeaMonkey CLOSED TREE

Comment 85

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/comm-central/rev/6b0dd3834439

Comment 86

[:aceman]

The menu-button patch pushed as
https://hg.mozilla.org/comm-central/rev/e595204ac239

Comment 87

[forever_love]

Hi, I see the bug is closed and there was bounty for fixing this bug. 
https://www.bountysource.com/issues/3493550-use-openstreetmap-for-maps-even-allow-the-user-to-choose-from-list-of-map-services

Person who fixed the issue, can claim to bountysource.com to get money. The amount is small, but still.

Comment 88

[:aceman]

Thanks for the notification. If possible, please forward that donation to https://donate.mozilla.org/en-US/thunderbird/about/ . Thanks!

Comment 89

[forever_love]

Hi, I'm not the baker. 
I've just found the bounty on the site and noticed that the bug got fixed, \
but the bounty was not paid out. 

As I understand to get money developer needs to contact bountysource.com platform and inform it that he has closed the issue. You can read it their FAQ in details:

https://github.com/bountysource/core/wiki/Frequently-Asked-Questions

Comment 90

[Pander]

The developer who fixed this bug must claim it himself or herself. Please do so as otherwise it piles up at Bounty Source and the people donating (even how small it is) get disappointed as the money doesn't go to the developers.

Comment 91

[Pander]

Another question is the following, for most Dutch postcodes this patch doesn't work. See https://github.com/openstreetmap/Nominatim/issues/1027 Perhaps a better way to call Nominatim is not to concatenate all the fields with commas, but use name parameters as is documented at https://wiki.openstreetmap.org/wiki/Nominatim#Parameters . What do you all think?

Comment 92

[:aceman]

I would like the structured arguments to Nominatim, if they work. Please open a new bug for that.