Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Split OMEMO plugin into view and headless components #3117

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

badrihippo
Copy link

@badrihippo badrihippo commented Jan 8, 2023

This commit replaces src/omemo with plugins/omemo-views and headless/plugins/omemo. The way I'm working on this is to keep everything on omemo-views for now, and then gradually move whatever possible out to the headless omemo.

Files to be tackled (more will be added as events warrant):

  • util
    • copy the functions whole
    • break up the functions which have UI and non-UI parts to them
  • index
    • create and register the headless omemo plugin separately
    • move headless event handlers to headless version
    • move over other headless dependency files as appropriate
  • miscellaneous small files which can mostly be copied over directly

Standard steps to finish before completing pull request:

  • Add a changelog entry for your change in CHANGES.md
  • When adding a configuration variable, please make sure to
    document it in docs/source/configuration.rst
  • Please add a test for your change. Tests can be run in the commandline
    with make check or you can run them in the browser by running make serve
    and then opening http://localhost:8000/tests.html.

This replaces 'plugins/omemo' with 'plugins/omemo-views' and
'headless/plugins/omemo'. Most of the original code is still in
the former, but the contents of 'util' has been moved to the
headless version. Still pending: moving 'index.js' and all the
other files in the plugin!
Copy link
Author

@badrihippo badrihippo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished moving over utils, but with a few potential loose ends. @jcbrand I've left four comments for the doubtful areas (tagging you in the first two but then I realised I can avoid spam-tagging by doing it once here 😅).

Let me know if I'm on the right track there? I think I've got the rest under control!

src/plugins/omemo-views/utils.js Outdated Show resolved Hide resolved
src/plugins/omemo-views/utils.js Show resolved Hide resolved
src/plugins/omemo-views/utils.js Show resolved Hide resolved
src/plugins/omemo-views/utils.js Outdated Show resolved Hide resolved
src/plugins/omemo-views/utils.js Fixed Show fixed Hide fixed
src/plugins/omemo-views/utils.js Fixed Show fixed Hide fixed
src/plugins/omemo-views/utils.js Fixed Show fixed Hide fixed
src/plugins/omemo-views/utils.js Fixed Show fixed Hide fixed
src/plugins/omemo-views/utils.js Fixed Show fixed Hide fixed
src/plugins/omemo-views/index.js Fixed Show fixed Hide fixed
@badrihippo
Copy link
Author

badrihippo commented Jan 11, 2023

@jcbrand thanks for the comments! Just letting you know I've seen them; will get onto it once I've processed index.js 👷

The 'omemo-views' plugin is still there, but most of the logic has
now been moved to the new headless 'omemo' plugin, with only the
UI part remaining in 'omemo-views'.
Turns out there was no headless component to these functions, so
nothing to be moved!
@@ -5,7 +5,7 @@ import log from '@converse/headless/log';
import range from 'lodash-es/range';
import omit from 'lodash-es/omit';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this file can also be moved to the headless plugin. Generally speaking, all models should go into headless, and OMEMOStore is a model.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, okay! All the tests passed but when I tried running in the browser it's not actually running. So let me debug that and then I'll get to this 🙂

@@ -0,0 +1,137 @@
/* global libsignal */
import log from '@converse/headless/log';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused import log.
import { _converse, converse, api } from '@converse/headless/core';
import { html } from 'lit';
import { isError } from '@converse/headless/utils/core.js';
import { isAudioURL, isImageURL, isVideoURL, getURI } from '@converse/headless/utils/url.js';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused import getURI.
uri,
start,
end,
obj_url: getAndDecryptFile(uri); // this is a promise

Check notice

Code scanning / CodeQL

Syntax error

Error: Unexpected token
@badrihippo
Copy link
Author

@jcbrand I have a problem: the omemo-views plugin seems to be working, but the headless/omemo does not.

Symptom: I get a converse.api.omemo.generateKeys() is not defined when keys are to be generated. This function is set during the the initialize() function of headless/omemo.

Debugging: I tried adding a test console.log inside the initialize() function, and as I suspected it wasn't running. (A similar console.log line inside omemo-views/initialize() printed properly.

I tried importing the headless plugin to src/headless/index.js as well as to src/plugins/omemo-views/index.js, which is how it done for other plugins (like chatbox-views) but that didn't help either. Any idea what I could be missing?

@jcbrand
Copy link
Member

jcbrand commented Jan 26, 2023

@badrihippo Yes, I think I know what the issue is.

You need to add converse-omemo to the list of headless plugins at src/headless/shared/constants.js.

That is necessary in order to register it as a pluggable.js plugin. Otherwise, even though you import it, it's not registered as a plugin and the initialize function doesn't get called.

@timothyerwin
Copy link

@jcbrand @badrihippo hi, I'm interested in using the converse headless but it doesn't support omemo? any eta on this pr to make it possible? thanks!

@badrihippo
Copy link
Author

@jcbrand @badrihippo hi, I'm interested in using the converse headless but it doesn't support omemo? any eta on this pr to make it possible? thanks!

Well, I guess this has become stale 🙁 and also, I've forgotten half of what I did. But let me start afresh and see if I can do a better job this time...

@Neustradamus
Copy link

@badrihippo: Any progress on it?

@badrihippo
Copy link
Author

@Neustradamus I'm afraid not 🙁 I think I got quite close, but I haven't had time to work on it since that last commit. I think I got pretty close, so if anyone is willing to clone and do the last few fixes to get it running that would help. (@timothyerwin would you be interested?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants