GNOME Bugzilla – Bug 660915
Blank lines in addresses prevent from applying changes
Last modified: 2015-03-05 08:36:10 UTC
If you inadvertently insert a line without filling in any address in nm-connections-editor's IPv4/IPv6 tabs, the "Save..." button is greyed out without any indication of the reason. This is confusing because it's hard to notice that a blank line is present at all without clicking on it. I've observed it can be a real problem for users while helping somebody on the phone. I suggest either: 1) Refuse to create blank lines. This simply means removing the line if empty when validating one of its cells. 2) Ignore blank lines when deciding to disable the "Save..." button. I think option 1) is better. It's consistent with e.g. what happens in the file chooser if you try to create a directory and don't edit the default filename: the new dir isn't created.
Confirming for Git. This could probably be fixed by just removing all blank lines whenever it is unfocused. It is already not easy to unfocus it before filling it in, but it's possible by e.g. clicking another tab.
I have pushed a branch that improves error highlighting of IP address/routes table - jk/editor-error-bg-color-bgo660915 The incorrect values are now marked with red background. (So a blank line will be red as well.) Also, note we have had on-the-fly checking with green/red background for some time. Unfortunately, it stopped working some time ago due to changes in Gtk theming. This has been fixed in master by f224c15 editor: fix displaying background color when highlighting errors
pushed fixup! for trailing whitespaces. cell_error_data_func() contains quite some duplicated code. Could you factor out some utility functions? Maybe a utils_set_cell_backgroud() function? + markup = g_strdup_printf ("<span background='%s'>%s</span>", color, value); must xml escape value. + invalid = !value || !*value || !parse_netmask (value, &prefix); + invalid = !value || !*value || !is_prefix_valid (value, NULL); is_prefix_valid() happily accepts !value || !*value arguments (good!) if you modifiy parse_netmask(), you can also omit the checks there. IMO functions like is_a(), is_valid(), try_parse() should accept NULL arguments. I dislike the inverse logic of the @invalid variable - invalid = value && *value && !nm_utils_ipaddr_valid (AF_INET6, value); + valid = !value || !*value || nm_utils_ipaddr_valid (AF_INET6, value); In the !invalid case, don't you have to clear markup too? } else - g_object_set (G_OBJECT (cell), "cell-background-set", FALSE, NULL); + g_object_set (G_OBJECT (cell), + "cell-background-set", FALSE, + "markup", NULL, + NULL); and what about the "if (!value || !*value) {" case?
(In reply to Thomas Haller from comment #3) > pushed fixup! for trailing whitespaces. > Squashed, thanks. > > cell_error_data_func() contains quite some duplicated code. Could you factor > out some utility functions? Maybe a utils_set_cell_backgroud() function? > Done, pushed a new commit. > > + invalid = !value || !*value || !parse_netmask (value, &prefix); > + invalid = !value || !*value || !is_prefix_valid (value, NULL); > > is_prefix_valid() happily accepts !value || !*value arguments (good!) > > if you modifiy parse_netmask(), you can also omit the checks there. IMO > functions like is_a(), is_valid(), try_parse() should accept NULL arguments. > Updated the functions. > > I dislike the inverse logic of the @invalid variable > > - invalid = value && *value && !nm_utils_ipaddr_valid (AF_INET6, > value); > + valid = !value || !*value || nm_utils_ipaddr_valid (AF_INET6, > value); > It's not the inverse logic. We *are* interested in invalid case here. > > In the !invalid case, don't you have to clear markup too? > > } else > - g_object_set (G_OBJECT (cell), "cell-background-set", FALSE, NULL); > + g_object_set (G_OBJECT (cell), > + "cell-background-set", FALSE, > + "markup", NULL, > + NULL); No, setting markup to NULL would clear the value. Branch repushed.
(In reply to Jiri Klimes from comment #4) > (In reply to Thomas Haller from comment #3) > > pushed fixup! for trailing whitespaces. > > > Squashed, thanks. hm, there are more trailing spaces... I missed them in my fixup. > > > > cell_error_data_func() contains quite some duplicated code. Could you factor > > out some utility functions? Maybe a utils_set_cell_backgroud() function? > > > Done, pushed a new commit. + markup = g_strdup_printf ("<span background='%s'>%s</span>", color, value); I still think @value must be XML escaped. > > + invalid = !value || !*value || !parse_netmask (value, &prefix); > > + invalid = !value || !*value || !is_prefix_valid (value, NULL); > > > > is_prefix_valid() happily accepts !value || !*value arguments (good!) > > > > if you modifiy parse_netmask(), you can also omit the checks there. IMO > > functions like is_a(), is_valid(), try_parse() should accept NULL arguments. > > > Updated the functions. I would squash the commits, as the latter two only fix the first one. otherwise looks good (didn't test it)
(In reply to Thomas Haller from comment #5) > (In reply to Jiri Klimes from comment #4) > > (In reply to Thomas Haller from comment #3) > > > pushed fixup! for trailing whitespaces. > > > > > Squashed, thanks. > > hm, there are more trailing spaces... I missed them in my fixup. > Fixed. > > > > > > cell_error_data_func() contains quite some duplicated code. Could you factor > > > out some utility functions? Maybe a utils_set_cell_backgroud() function? > > > > > Done, pushed a new commit. > > + markup = g_strdup_printf ("<span background='%s'>%s</span>", color, > value); > > I still think @value must be XML escaped. > It worked in this case just fine because the cells cannot contain pango/XML control characters. Anyway, I have changed g_strdup_printf() to g_markup_printf_escaped(). > > I would squash the commits, as the latter two only fix the first one. > Done. Committed to master: 975181d editor: improve color-indication for bad addresses and routes (bgo #660915)