On 01/02/2019 05:50 AM, Martin Hundebøll wrote: > Let the user/connection-manager decide what to do with duplicate apn > entries instead of bailing out with an error. > --- >   plugins/provision.c | 2 +- >   1 file changed, 1 insertion(+), 1 deletion(-) While seemingly trivial, this actually has a huge impact on user experience.  The problem is that there's no good meta-data for the user to make the decision as to which service is the one they want.

Also, I'm not sure how this impacts connman?  In theory it is supposed to auto-activate the internet context, and if there are multiple one of these, I'm not sure if it gets confused?

The typical guidance is that if a database contains potential duplicates, then a UI Wizard is needed to guide the user to choose the appropriate one and provision the context that way.

Alternatively, provide a provisioning database without conflicts, perhaps specific to the users / operators you're targeting.

On 01/04/2019 08:03 AM, Martin Hundebøll wrote: > On 02/01/2019 19.36, Denis Kenzior wrote: >> On 01/02/2019 05:50 AM, Martin Hundebøll wrote: >>> Let the user/connection-manager decide what to do with duplicate apn >>> entries instead of bailing out with an error. >>> --- >>>   plugins/provision.c | 2 +- >>>   1 file changed, 1 insertion(+), 1 deletion(-) >> >> While seemingly trivial, this actually has a huge impact on user >> experience.  The problem is that there's no good meta-data for the >> user to make the decision as to which service is the one they want. > > Yes. The lack of meta-data makes it difficult to make a good decision. > In the case of the mobile-broadband-provider-info the <name> element > from the parent provider element would be helpful: > > diff --git a/plugins/mbpi.c b/plugins/mbpi.c > index 433f1b55..69ffea8a 100644 > --- a/plugins/mbpi.c > +++ b/plugins/mbpi.c > @@ -56,6 +56,7 @@ struct gsm_data { >          GSList *apns; >          gboolean match_found; >          gboolean allow_duplicates; > +       char *current_name; >   }; > >   struct cdma_data { > @@ -401,6 +402,9 @@ static void gsm_end(GMarkupParseContext *context, > const gchar *element_name, >          if (!ap->username || !ap->password) >                  ap->auth_method = OFONO_GPRS_AUTH_METHOD_NONE; > > +       if (!ap->name && gsm->current_name) > +               ap->name = g_strdup(gsm->current_name); > + >          if (gsm->allow_duplicates == FALSE) { >                  GSList *l; > > @@ -503,7 +507,12 @@ static void > toplevel_gsm_start(GMarkupParseContext *context, >   { >          struct gsm_data *gsm = userdata; > > -       if (g_str_equal(element_name, "gsm")) { > +       if (g_str_equal(element_name, "name")) { > +               g_free(gsm->current_name); > +               gsm->current_name = NULL; > +               g_markup_parse_context_push(context, &text_parser, > +                                               &gsm->current_name); > +       } else if (g_str_equal(element_name, "gsm")) { >                  gsm->match_found = FALSE; >                  g_markup_parse_context_push(context, &gsm_parser, gsm); >          } else if (g_str_equal(element_name, "cdma")) > @@ -514,9 +523,16 @@ static void toplevel_gsm_end(GMarkupParseContext > *context, >                                          const gchar *element_name, >                                          gpointer userdata, GError > **error) >   { > +       struct gsm_data *gsm = userdata; > + >          if (g_str_equal(element_name, "gsm") || > -                       g_str_equal(element_name, "cdma")) > +                       g_str_equal(element_name, "cdma") || > +                       g_str_equal(element_name, "name")) >                  g_markup_parse_context_pop(context); > +       else if (g_str_equal(element_name, "provider")) { > +               g_free(gsm->current_name); > +               gsm->current_name = NULL; > +       } >   } > >   static const GMarkupParser toplevel_gsm_parser = { > I don't really mind taking a patch like this since it at least gives some basic name to the context if the <name> tag is missing inside the <apn> tag.  But...  I don't see how this helps you at all?  If you have duplicates, then you just get both contexts named the same way.  Maybe you're lucky and one context has a name and the other can use the provider name.  But still, not great user experience.

Also note that mbpi doesn't really make it clear whether any of these names are localized or need to be translated.  oFono doesn't do translations, any printable strings are assumed to be localized by the SIM.

I guess you can tell that I'm no fan of mbpi.  If you're doing something serious, you're much better off looking into using something else.

> >> Also, I'm not sure how this impacts connman?  In theory it is >> supposed to auto-activate the internet context, and if there are >> multiple one of these, I'm not sure if it gets confused? > > I haven't looked at connman (yet). Couldn't connman create an empty > internet context if it gets confused? (just like the one ofono creates > when provision fails.) I don't think so?  But this is a conversation you need to have with the ConnMan folks.

>> The typical guidance is that if a database contains potential >> duplicates, then a UI Wizard is needed to guide the user to choose >> the appropriate one and provision the context that way. > > Yes, but wouldn't that require access to the duplicate entries to be > able to create the wizard? No, just have the wizard parse mbpi directly.

>> Alternatively, provide a provisioning database without conflicts, >> perhaps specific to the users / operators you're targeting. > > That removes the convenience from relying on the > mobile-broadband-provider-info maintained by upstream. > Not a big loss in my opinion ;)