Use KDE Connect's verification codes #1148

strugee · 2021-07-29T09:08:01Z

Still TODO:

Diagnose segfaults in the daemon
Figure out if the UTF-8 errors are a problem
Display verification codes in the notification (right now you need to have the preferences window open)
Clean up the style
Clean up the .ui file diff that Glade made a mess of
Fix the verification code computation so it actually works

Some notes on the segfaults: they seem to happen during the pairing process. I think there's a race condition or something like that during the verification code computation. Most of the time I just get Segmentation fault (core dumped), but occasionally I get a longer assertion error:

GLib:ERROR:../glib/gchecksum.c:1618:g_checksum_update: code should not be reached
Bail out! GLib:ERROR:../glib/gchecksum.c:1618:g_checksum_update: code should not be reached
Aborted (core dumped)

The UTF-8 errors come from the preferences pane and all look like this:

(gsconnect-preferences:335958): Gjs-CRITICAL **: 01:53:39.208: JS ERROR: TypeError: malformed UTF-8 character sequence at offset 0
@/var/home/alex/.local/share/gnome-shell/extensions/[email protected]/gsconnect-preferences:114:21

strugee · 2021-07-29T09:12:45Z

Also, I used Unicode 🔑 instead of an icon because I couldn't find a suitable icon in what Glade said was available. If there's a better way, let me know.

One more thing - this is basically my first time writing anything approaching GTK/GObject/etc. code, so please review with extra scrutiny. I might have broken everything!

andyholmes · 2021-07-29T23:58:53Z

Diagnose segfaults in the daemon

You can run the daemon with GDB if necessary. Let me know if that's something you want help with and after I can add it to the contributing document.

Figure out if the UTF-8 errors are a problem

These are fine to leave and not related to your PR. It's just a corner-case in GJS and GDBus related to virtual functions.

Display verification codes in the notification (right now you need to have the preferences window open)

I think that the combined verification code is what needs to be shown in the notification, at least I think that's what Android does. I think the older keys are effectively being deprecated.

ferdnyc

Hey, I know this has been here a good long while already, but I jotted down a few things as I was perusing it just now.

ferdnyc · 2022-01-20T22:58:46Z

data/ui/preferences-device-panel.ui

+                    </child>
+                    <child>
+                      <object class="GtkButton">
+                        <property name="label" translatable="yes" context="Send a pair request to the device">Request pairing</property>


Suggested change

<property name="label" translatable="yes" context="Send a pair request to the device">Request pairing</property>

<property name="label" translatable="yes" comment="Send a pair request to the device">Request pairing</property>

This should be a translator comment= property, rather than a context=. Contexts are typically used in a more structured way, usually as a hierarchy based on things like class names or high-level UI objects.¹ (For example, obvious picks for the context of these messages might be GSConnectPreferencesDevicePanel or Preferences|Device.)

Comments, OTOH, are for supplying free-form hints about intended meaning to assist translators, as you've done. 👍

Notes

(Contexts are used to categorize strings within the code, so that the same source-language string can be used in different situations where it would potentially get translated differently. Like, "Print" could be the title of a dialog, the label for an action button, or it may be used to refer to the output of a printer. Those might be three completely different words in some languages. So there would be three different translatable "Print" strings in the translation master, each tagged with a different context.)

ferdnyc · 2022-01-20T23:07:42Z

data/ui/preferences-device-panel.ui

                    <child>
-                      <object class="GtkLabel">
+                      <!-- n-columns=3 n-rows=3 -->


Were these dimension comments inserted automatically by some tool, or did you place them manually?

I don't love embedding narration into code, especially as hidden comment text. Simple reason: I have never, ever seen a narrative comment get updated when subsequent changes to the code turn it into a complete LIE. Not one single time. Any narrative comment that's more than a year or two old, or is located in a file that's had more than 20 commits touch it since then, has a roughly 99% chance of being completely false. And the only thing worse than a comment that describes code, is a comment that describes code incorrectly.

...But if some tool manages those, then I suppose it's fine. At least when that tool is used to make changes, they'll presumably be brought up to date.

ferdnyc · 2022-01-21T00:11:12Z

src/service/device.js

+        // let b = ByteArray.toGBytes(remoteCert.certificate);
+        let a = ByteArray.toGBytes(ByteArray.fromString('a'));
+        let b = ByteArray.toGBytes(ByteArray.fromString('b'));
+        // Lexographic a < b comparison; this is what the < operator on QByteArray does


Nit: "lexicographic"

ferdnyc · 2022-01-21T00:46:37Z

src/service/device.js

+        verifyCode = checksum.get_string();
+        checksum.free();
+
+        return verifyCode;


Your comment mentioned segfaults... according to the Checksum docs...

get_string()
Returns:
(String) — the hexadecimal representation of the checksum. The
returned string is owned by the checksum and should not be modified
or freed.

I'm gonna go out on a limb and say that means it's probably NOT OK to access the Checksum-owned string after freeing the Checksum. Perhaps, to be safe...?

verifyCode = GLib.strdup(checksum.get_string());

(Note that you'd then have to free the newly-allocated verifyCode String at some later point, once you're finished with it.)

Well, that is bizarre so I've asked about it in the #javascript channel. SpiderMonkey should be able to handle this without the GLib.strdup(), but I'm surprise that function is even in our bindings and that it seems to work! GLib.free() is definitely a no-go in GJS, so I'm not sure if this will leak memory or if SM can free it.

@andyholmes Heh, always fun when the soft underbelly of a native C API is exposed to a higher-level lanugage. Just like Python's lazy garbage collector frees developers from worrying much about allocation lifetimes... in theory.

Until you find yourself working with SWIG-generated C/C++ library bindings in Python. Then, unless both the native library and its bindings are really meticulous, you definitely start to care about your object lifetimes. In C++, not properly cleaning up your allocations "merely" leaks memory. But if Python's gc goes stomping around, cleaning up for you, invalidating pointers it hasn't been told are still very much in use over on the native side, You're Gonna Have A Bad Time™.

ferdnyc · 2022-01-21T00:47:57Z

src/preferences/device.js

+        // Action setup
        this._setupActions();


Phew, there's that mystery solved! 😈

Lol, I recall feeling the same way writing it. But it looked out of place otherwise 😂

ferdnyc · 2022-10-24T00:27:56Z

Closing this as completed, since it was brought home by @daniellandau in #1493 and is now merged. Thanks @strugee !

ferdnyc · 2022-10-24T00:37:57Z

Whoops. Premature close, as there's more to this PR than what went in with #1493. Reopening, and attempting to reconcile with the current main branch HEAD.

Fixes GSConnect#1091

ferdnyc · 2022-10-24T03:23:01Z

attempting to reconcile with the current main branch HEAD.

That actually wasn't hard at all, in terms of actual change conflicts. Whether the code on this branch is still compatible with the main source is another matter, which I guess the CI will at least help us determine...

strugee · 2023-07-28T01:33:46Z

Hey, sorry for disappearing - life got in the way :-)

Glad to hear my work was useful for #1493.

strugee force-pushed the verify-code branch from f5b6558 to 5731b63 Compare July 29, 2021 09:11

strugee force-pushed the verify-code branch from 5731b63 to a8f73c6 Compare July 29, 2021 09:15

ferdnyc reviewed Jan 21, 2022

View reviewed changes

andyholmes added the help wanted An issue that needs contributors label Aug 5, 2022

daniellandau mentioned this pull request Oct 23, 2022

Calculate a verification key that matches KDE/Android #1493

Merged

ferdnyc closed this Oct 24, 2022

ferdnyc reopened this Oct 24, 2022

Use KDE Connect's verification codes

1bb11b2

Fixes GSConnect#1091

ferdnyc force-pushed the verify-code branch from a8f73c6 to 1bb11b2 Compare October 24, 2022 03:21

Merge branch 'main' into verify-code

372a7d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use KDE Connect's verification codes #1148

Use KDE Connect's verification codes #1148

strugee commented Jul 29, 2021

strugee commented Jul 29, 2021

andyholmes commented Jul 29, 2021

ferdnyc left a comment

ferdnyc Jan 20, 2022

ferdnyc Jan 20, 2022

ferdnyc Jan 21, 2022

ferdnyc Jan 21, 2022

ferdnyc Jan 21, 2022

andyholmes Jan 21, 2022

ferdnyc Jan 22, 2022

ferdnyc Jan 21, 2022

strugee Jul 28, 2023

ferdnyc commented Oct 24, 2022

ferdnyc commented Oct 24, 2022

ferdnyc commented Oct 24, 2022

strugee commented Jul 28, 2023

	<property name="label" translatable="yes" context="Send a pair request to the device">Request pairing</property>
	<property name="label" translatable="yes" comment="Send a pair request to the device">Request pairing</property>

Use KDE Connect's verification codes #1148

Are you sure you want to change the base?

Use KDE Connect's verification codes #1148

Conversation

strugee commented Jul 29, 2021

strugee commented Jul 29, 2021

andyholmes commented Jul 29, 2021

ferdnyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferdnyc commented Oct 24, 2022

ferdnyc commented Oct 24, 2022

ferdnyc commented Oct 24, 2022

strugee commented Jul 28, 2023