OK, pushed three "setup" branches for early review:

  - danw/tui-filepicker-bgo719768 adds a file picker dialog widget to
    nmt-newt, which will be used for picking cert/key/CA files.

  - danw/cert-apis-bgo719768 adds nm-utils APIs for recognizing certificate
    and key files (and has a few other cert-related patches too). (The idea
    comes from nm-connection-editor, although most of the code does not.)

  - danw/tui-sections-bgo719768 changes how the tui editor is built.
    The background is that VPNs are going to have multiple "sections" in
    the editor (eg, "OpenVPN", "OpenVPN: Advanced", "OpenVPN: Security",
    "OpenVPN: TLS Authentication", "OpenVPN: Proxies"; the first corresponds
    to the main VPN page in nm-connection-editor, and will be shown by
    default. The others correspond to tabs in the "Advanced" dialog and will
    be collapsed by default.) Anyway, things work better if there is a single
    object to coordinate the different sections, so this branch effectively
    renames NmtEditorPage to NmtEditorSection, and creates a new
    NmtEditorPage that just manages one or more NmtEditorSections.

For danw/vpn-prep-bgo719768:

> libnm: rename NMVpnPluginUiInterface, add to NetworkManager.h

While you're at it, add some 'gpointer padding[8]' to NMVpnEditorPluginInterface and NMVpnEditor.  Looks sensible & good though.

(In reply to comment #2)
> While you're at it, add some 'gpointer padding[8]' to
> NMVpnEditorPluginInterface and NMVpnEditor.

Interfaces don't need padding because you can make them bigger without breaking API (and the extra fields automatically get initialized to "0").

pushed to master

danw/tui-filepicker-bgo719768:


>> tui: miscellaneous NmtNewt API improvements

 nmt_newt_listbox_get_key (NmtNewtListbox *listbox,
                           int             row)
 update_active_internal (NmtNewtListbox *listbox,
                         int             new_active)

at several places, the index is a signed int. How about g_return_if_fail(row >= 0); or even better: make it guint.


>> tui: add NmtNewtFileDialog, NmtNewtFileButton

+         /* @path is neither UTF-8 nor locale encoding, and does not refer to
+          * an actual file. Yay.
+          */
+         return g_strdup ("???");

shouldn't you return NULL, so that the caller can do error handling (if he wishes to do so). Or alternatively, g_strescape()? Or is there no function that replaces non-UTF8 characters with some special character?

Anyway: wow, support for non-UTF-8 filenames. Who would have thought... :)





+    return g_object_new (NMT_TYPE_NEWT_FILE_DIALOG,
+                         // FIXME "escape-exits", TRUE,
+                         NULL);


+static void
+update_selection (NmtNewtFileDialog *dialog)
+{
...
+    dirname = g_path_get_dirname (priv->selection);
+    if (strcmp (dirname, priv->cwd) != 0) {
...
+         if (info && !strcmp (basename, g_file_info_get_name (info))) {

Probably this is correct, but it's non-obvious to me why they cannot be NULL. Especially priv->cwd. Use g_strcmp0()?



rebuild_dialog():

g_return_if_fail (priv->cwd);


-         nmt_newt_listbox_append (priv->list, display_name, info);
+         nmt_newt_listbox_append_full (priv->list, display_name, info);


+ iter = g_file_enumerate_children (dir,
...
+    /* FIXME */
+    g_assert_no_error (error);

maybe just assume empty list?




/**
 * NmtNewtFilePicker:cwd:
 *
 * You cannot change this while the widget is realized.

I think nmt_newt_file_dialog_set_cwd() should g_return if trying to modify cwd of a realized item. Also because the dialog depends on being this one as expected.



transform_cwd_for_label():

at other places you care about UTF-8, but here you just count the number of bytes.




Rest looks good

(In reply to comment #4)
> +         return g_strdup ("???");
> 
> shouldn't you return NULL, so that the caller can do error handling (if he
> wishes to do so).

At the moment, the caller does not wish to do so. :)

> Or alternatively, g_strescape()? Or is there no function that
> replaces non-UTF8 characters with some special character?

You can hack one up using g_utf8_validate (since it returns a pointer to the first invalid character), and GLib has one internally that people have suggested exporting, but basically, no. If the function existed, I'd use it here, but since this fallback/error only applies to the case where the filename (a) is not UTF-8, (b) is not in the user's locale encoding, and (c) doesn't actually refer to an existing file (since gio will escape/mangle the filename for us if it does exist), it didn't seem worth doing any extra work for.

> Anyway: wow, support for non-UTF-8 filenames. Who would have thought... :)

Filenames are assumed to be in the locale encoding, and some people use non-UTF-8 locales. (This is why gobject-introspection distinguishes (type utf8) and (type filename) for strings.)

danw/cert-apis-bgo719768:

>> libnm-core: tweak crypto.c APIs

parse_old_openssl_key_file(): I know, we often don't do that, but could we set @out_key_type even on failure? I always find this pattern error prone.

OTOH, crypto_decrypt_openssl_private_key_data() always sets the out variable.



crypto_verify_private_key_data(): can now drop @ktype.



>> libnm-core: add nm_utils_file_is_certificate() and _file_is_private_key()

nm_utils_file_is_private_key():
nm_utils_file_is_certificate():

checks for file_has_extension(). Usually I dislike it if an application insists for a file to having a certain extension. I think, the should just check the content.



Rest looks good to me

branch danw/tui-sections-bgo719768 looks good to me

(In reply to comment #6)
> danw/cert-apis-bgo719768:

> parse_old_openssl_key_file(): I know, we often don't do that, but could we set
> @out_key_type even on failure? I always find this pattern error prone.

ok

> crypto_verify_private_key_data(): can now drop @ktype.

well, parse_old_openssl_key_file() assumes you're not passing NULL for the out_key_type (which would now be more annoying to change since it's now setting the value even on failure :). So I left it there.

> >> libnm-core: add nm_utils_file_is_certificate() and _file_is_private_key()
> 
> nm_utils_file_is_private_key():
> nm_utils_file_is_certificate():
> 
> checks for file_has_extension(). Usually I dislike it if an application insists
> for a file to having a certain extension. I think, the should just check the
> content.

That's just preserving the existing behavior from the old functions in network-manager-applet.

And in particular, these functions will get used as file dialog filter functions, so you don't want them opening and scanning every single file. (And by having the extension checking in libnm rather than in nm-applet, we ensure we get consistent behavior between nm-connection-editor and nmtui.)


pushed danw/cert-apis-bgo719768 and danw/tui-sections-bgo719768. (The tui filepicker stuff is still uncommitted.)

moving to nm-1-4

bugzilla.gnome.org is being shut down in favor of a GitLab instance. 
We are closing all old bug reports and feature requests in GNOME Bugzilla which have not seen updates for a long time.

If you still use NetworkManager and if you still see this bug / want this feature in a recent and supported version of NetworkManager, then please feel free to report it at https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/

Thank you for creating this report and we are sorry it could not be implemented (workforce and time is unfortunately limited).