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

ElectronPlatform: Add support for a event index using Seshat. #11125

Merged
merged 32 commits into from
Nov 26, 2019

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Oct 11, 2019

This is the platform specific part of our E2EE search support. The ElectronPlatform is extended to make use of Seshat.

@t3chguy
Copy link
Member

t3chguy commented Oct 11, 2019

The flow type annotations in ElectronPlatform are a bit un-useful with {}

Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks, looks like a good starting point!

If you'd like to work towards merging this before we have a build process ready for native modules, then we should do something like:

  • Test for the native node module(s) needed with runtime checks
  • Do not add the native node module as an app dependency (instead record what should be added in a doc)

The push-to-talk PR (which is in a similar boat) has a draft of docs around native node modules, so perhaps we can describe the manual steps needed in something like that.

As for the seshat backend itself, as you've seen, I filed various issues over there, but most are smaller nits that can be addressed whenever. I would like to see the following resolved in some way or another before merging integration into Riot:

@@ -5,11 +5,17 @@
"version": "1.4.2",
"description": "A feature-rich client for Matrix.org",
"author": "New Vector Ltd.",
"scripts": {
"build": "electron-build-env --electron 6.0.3 neon build seshat-node --release",
"postinstall": "yarn build"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, so this is just to call the line above...?

@@ -5,11 +5,17 @@
"version": "1.4.2",
"description": "A feature-rich client for Matrix.org",
"author": "New Vector Ltd.",
"scripts": {
"build": "electron-build-env --electron 6.0.3 neon build seshat-node --release",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should really try hard to find a way to build that doesn't duplicate the Electron version in another place, as we'll surely forget to keep them synchronised. Maybe we need a wrapper script to pull it out of the main package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing doesn't even work with the version passed as is. It seems to be only necessary if you need to rebuild seshat for some reason (e.g. if you're linking it from your dev folder) anyways.

Since we're adding run-time checks for seshat anyways it will be safe to remove it as well.

"png-to-ico": "^1.0.2"
"png-to-ico": "^1.0.2",
"make-dir": "^3.0.0",
"seshat-node": "^0.2.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be quite nice to have this renamed to something without the redundant node instead. See matrix-org/seshat#9.

let p = path.normalize(path.join(eventStorePath, args[0]));
try {
await makeDir(p);
eventIndex = new seshat(p);
Copy link
Collaborator

Choose a reason for hiding this comment

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

A common JS convention is for new-able things to start with a capital letter. I suppose we may not have a linting rule enforcing that at the moment, but I suggest changing the require to call it Seshat instead, unless there's a strong reason to always use the lowercase name.

@@ -149,6 +155,17 @@ autoUpdater.on('update-downloaded', (ev, releaseNotes, releaseName, releaseDate,
ipcMain.on('ipcCall', async function(ev, payload) {
if (!mainWindow) return;

const send_error = (id, e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use camel-case naming:

Suggested change
const send_error = (id, e) => {
const sendError = (id, e) => {

(We should have linting rules for this, but they seem not to have fired here... 🤔)

@@ -39,6 +39,8 @@ const { migrateFromOldOrigin } = require('./originMigrator');

const windowStateKeeper = require('electron-window-state');
const Store = require('electron-store');
const seshat = require('seshat-node');
Copy link
Collaborator

Choose a reason for hiding this comment

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

As part of attempting to land this without a path to build it, let's test at runtime for its existence with a try / catch.

@poljar
Copy link
Contributor Author

poljar commented Nov 13, 2019

This should be ready for another look. The thing that's missing is switching from seshat-node to matrix-seshat, enabling encryption, and removing the stuff from package.json. Will be a trivial fix, once I get matrix-seshat published.

@poljar poljar requested a review from jryans November 13, 2019 16:27
@poljar
Copy link
Contributor Author

poljar commented Nov 14, 2019

The logout logic, and therefore event index deletion required a bit more changes here since I wrongly scoped our event index to a user id.

Should now be ready.

Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks for the continued work here. It looks reasonable enough, but I'd like to review the final form with dep removed, documentation to enable, etc. so I can check the approach, see how it aligns with the push-to-talk PR, etc.

If you'd like another review Thursday - Friday while I am away, feel free to request review from the riot-web team, or else we can wait until I am back next week.

@poljar poljar requested a review from jryans November 25, 2019 09:39
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

I realise you copied some of this text from the Push-to-talk PR. I hadn't reviewed it in depth over there yet, as it was still unclear how it would all come together...

Since it looks like you'll be the first to merge with native modules, that means the detailed text review is happening here. Apologies for all my copy edits, but I want make sure we convey the right message in these docs.

docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
Comment on lines +31 to +44
If for some reason recompilation of Seshat is needed, e.g. when using a
development version of Seshat using `yarn link`, or if the initial compilation was
done for the wrong electron version, Seshat can be recompiled with the
`electron-build-env` tool. Again from the `electron_app/` directory:

yarn add electron-build-env

Recompiling Seshat itself can be done like so:

yarn run electron-build-env -- --electron 6.1.1 -- neon build matrix-seshat --release`

Please make sure to include all the `--` as well as the `--release` command line
switch at the end. Modify your electron version accordingly depending on the
version that is installed on your system.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, so is electron-build-env needed even for the first installation of the matrix-seshat Node module, or is only a rebuild helper?

If it's only used for rebuilds, I wonder if we can find some way to repeat the build that happens at install time without this extra tool somehow, such as (cd node_modules/matrix-seshat; yarn run install) or similar.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also noticed that the Electron docs suggest electron-rebuild, is that of any use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's only needed to rebuild it, but I'm not 100% on that.

The neon docs state this:

We are working on adding Neon support to electron-rebuild, so you'll be able to just drop Neon dependencies into your app like any other. For now, there's a tool called electron-build-env that you can use for building any Neon dependencies of your Electron app.

As far as I can tell this has not changed as of yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After looking around a bit, it's quite difficult to follow what's actually recommended and also matches what we need here. 😓 I don't want to block all this work on sorting it out though, so I filed a separate issue to improve this in a future step.

package.json Outdated Show resolved Hide resolved
@@ -27,7 +27,8 @@
"feature_sas": "labs",
"feature_room_breadcrumbs": "labs",
"feature_state_counters": "labs",
"feature_many_integration_managers": "labs"
"feature_many_integration_managers": "labs",
"feature_event_indexing": "labs"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also add a short explanation of this flag in the labs docs as per the guide.

docs/native-node-modules.md Show resolved Hide resolved
Comment on lines +31 to +44
If for some reason recompilation of Seshat is needed, e.g. when using a
development version of Seshat using `yarn link`, or if the initial compilation was
done for the wrong electron version, Seshat can be recompiled with the
`electron-build-env` tool. Again from the `electron_app/` directory:

yarn add electron-build-env

Recompiling Seshat itself can be done like so:

yarn run electron-build-env -- --electron 6.1.1 -- neon build matrix-seshat --release`

Please make sure to include all the `--` as well as the `--release` command line
switch at the end. Modify your electron version accordingly depending on the
version that is installed on your system.
Copy link
Collaborator

Choose a reason for hiding this comment

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

After looking around a bit, it's quite difficult to follow what's actually recommended and also matches what we need here. 😓 I don't want to block all this work on sorting it out though, so I filed a separate issue to improve this in a future step.

@poljar poljar requested a review from jryans November 26, 2019 17:18
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

As always, thanks for working through all the nits and copy edits here! 😁

Looks like a few conflicts to resolve, but should be ready to merge. Assuming GH will let you, it's fine to merge without another review once those are resolved.

docs/labs.md Outdated
@@ -49,3 +49,11 @@ That's it. Now should see your new counter under the header.
## Multiple integration managers (`feature_many_integration_managers`)

Exposes a way to access all the integration managers known to Riot. This is an implementation of [MSC1957](https://github.com/matrix-org/matrix-doc/pull/1957).

## Event indexing or E2EE search support using Seshat (`feature_event_indexing`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably "and" is more natural since Seshat supports both.

Suggested change
## Event indexing or E2EE search support using Seshat (`feature_event_indexing`)
## Event indexing and E2EE search support using Seshat (`feature_event_indexing`)

@poljar poljar merged commit e5956de into develop Nov 26, 2019
@jryans jryans added the phase:1 label Feb 10, 2020
@poljar poljar deleted the poljar/seshat-pr branch June 21, 2020 17:56
su-ex added a commit to SchildiChat/element-web that referenced this pull request Feb 22, 2024
* Remove `feature_favourite_messages` as it is has been abandoned for now ([\#11097](matrix-org/matrix-react-sdk#11097)). Fixes element-hq#25555.
* Don't setup keys on login when encryption is force disabled ([\element-hq#11125](matrix-org/matrix-react-sdk#11125)). Contributed by @kerryarchibald.
* OIDC: attempt dynamic client registration ([\element-hq#11074](matrix-org/matrix-react-sdk#11074)). Fixes element-hq#25468 and element-hq#25467. Contributed by @kerryarchibald.
* OIDC: Check static client registration and add login flow ([\element-hq#11088](matrix-org/matrix-react-sdk#11088)). Fixes element-hq#25467. Contributed by @kerryarchibald.
* Improve message body output from plain text editor ([\element-hq#11124](matrix-org/matrix-react-sdk#11124)). Contributed by @alunturner.
* Disable encryption toggle in room settings when force disabled ([\element-hq#11122](matrix-org/matrix-react-sdk#11122)). Contributed by @kerryarchibald.
* Add .well-known config option to force disable encryption on room creation ([\element-hq#11120](matrix-org/matrix-react-sdk#11120)). Contributed by @kerryarchibald.
* Handle permalinks in room topic ([\element-hq#11115](matrix-org/matrix-react-sdk#11115)). Fixes element-hq#23395.
* Add at room avatar for RTE ([\element-hq#11106](matrix-org/matrix-react-sdk#11106)). Contributed by @alunturner.
* Remove new room breadcrumbs ([\element-hq#11104](matrix-org/matrix-react-sdk#11104)).
* Update rich text editor dependency and associated changes ([\element-hq#11098](matrix-org/matrix-react-sdk#11098)). Contributed by @alunturner.
* Implement new model, hooks and reconcilation code for new GYU notification settings ([\element-hq#11089](matrix-org/matrix-react-sdk#11089)). Contributed by @justjanne.
* Allow maintaining a different right panel width for thread panels ([\element-hq#11064](matrix-org/matrix-react-sdk#11064)). Fixes element-hq#25487.
* Make AppPermission pane scrollable ([\element-hq#10954](matrix-org/matrix-react-sdk#10954)). Fixes element-hq#25438 and element-hq#25511. Contributed by @luixxiul.
* Integrate compound design tokens ([\element-hq#11091](matrix-org/matrix-react-sdk#11091)). Fixes vector-im/internal-planning#450.
* Don't warn about the effects of redacting state events when redacting non-state-events ([\element-hq#11071](matrix-org/matrix-react-sdk#11071)). Fixes element-hq#8478.
* Allow specifying help URLs in config.json ([\element-hq#11070](matrix-org/matrix-react-sdk#11070)). Fixes element-hq#15268.
* Fix error when generating error for polling for updates ([\element-hq#25609](element-hq#25609)).
* Fix spurious notifications on non-live events ([\element-hq#11133](matrix-org/matrix-react-sdk#11133)). Fixes element-hq#24336.
* Prevent auto-translation within composer ([\#11114](matrix-org/matrix-react-sdk#11114)). Fixes element-hq#25624.
* Fix caret jump when backspacing into empty line at beginning of editor ([\#11128](matrix-org/matrix-react-sdk#11128)). Fixes element-hq#22335.
* Fix server picker not allowing you to switch from custom to default ([\element-hq#11127](matrix-org/matrix-react-sdk#11127)). Fixes element-hq#25650.
* Consider the unthreaded read receipt for Unread dot state ([\element-hq#11117](matrix-org/matrix-react-sdk#11117)). Fixes element-hq#24229.
* Increase RTE resilience ([\element-hq#11111](matrix-org/matrix-react-sdk#11111)). Fixes element-hq#25277. Contributed by @alunturner.
* Fix RoomView ignoring alias lookup errors due to them not knowing the roomId ([\element-hq#11099](matrix-org/matrix-react-sdk#11099)). Fixes element-hq#24783 and element-hq#25562.
* Fix style inconsistencies on SecureBackupPanel ([\element-hq#11102](matrix-org/matrix-react-sdk#11102)). Fixes element-hq#25615. Contributed by @luixxiul.
* Remove unknown MXIDs from invite suggestions ([\element-hq#11055](matrix-org/matrix-react-sdk#11055)). Fixes element-hq#25446.
* Reduce volume of ring sounds to normalised levels ([\element-hq#9143](matrix-org/matrix-react-sdk#9143)). Contributed by @JMoVS.
* Fix slash commands not being enabled in certain cases ([\element-hq#11090](matrix-org/matrix-react-sdk#11090)). Fixes element-hq#25572.
* Prevent escape in threads from sending focus to main timeline composer ([\element-hq#11061](matrix-org/matrix-react-sdk#11061)). Fixes element-hq#23397.
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.

5 participants