Ability to join an arbitrary room and use it as a target for higlights #11 by Stvad · Pull Request #28 · DanilaFe/matrix-highlight

Stvad · 2023-01-03T00:18:18Z

There are two TODO sections in the code that need additional discussion.
I'm also not entirely sure I handled the UI/state transitions as intended and would appreciate double-checking on that!

upgrade matrix client minor version
Add "watch" script for faster iteration
Add webextension plugin to enable auto-reload in Chrome to improve iteration speed.
Readme dev instructions update
Fix chrome build issue (non-ascii symbols in minified version)
Join and configure arbitrary room. Iteration 1 (works but has issues) Allow storing highlights on arbitrary rooms #11

…eration speed. It also can provide better facilities for manifest manegement, but not using that now

Stvad · 2023-01-03T22:09:08Z

"fixes" #27 too

DanilaFe · 2023-01-04T02:24:01Z

Thanks for your work!

I don't have time to review the whole thing today, but I am somewhat concerned about the added uses of !. Ideally, we wouldn't be using escape hatches for the type system. If this is due to an SDK upgrade (I don't see anh other causes at first glance) I'll need to think about this some more.

Otherwise, about the WebExtension plugin: we're still at manifest V2 with this change, and thus barred from the chrome web store?

Stvad · 2023-01-04T02:28:50Z

the non-null assertions are indeed because of the SDK upgrade and presumably them improving their types. I agree that ideally we should properly deal with that, but the current change doesn't change things functionally - just highlights that there is a problem we need to deal with 🤷‍♂️

Also, it'd be great to update to the latest major sdk version at some point!

WebExtension plugin: we're still at manifest V2 with this change, and thus barred from the chrome web store?

Yes, though it should make it easier to write a manifest that would be cross-compatible between v2 and v3 (it allows specifying browser-specific fields)

…anilaFe#11

Stvad · 2023-01-17T03:01:17Z

Hi, checking in on this! :)

DanilaFe · 2023-01-21T00:39:18Z

Hey @Stvad, sorry about the delay. I really appreciate the work you're putting in. I work a full-time software job nowadays, and in all honesty I'm a little too tired most of the time to do much else software related.

Now, my thinking re: the ! and Room? is that I knew what I was doing when I didn't check for null - I realized that the types were improperly assigned, but I was lazy and didn't bother writing the checks. Now that the checks are finally required, that little laziness debt is to be repayed. However, since you didn't introduce the possible null bugs (I did 😄 ), I think it's fine to merge the ! versions. I'll give the code another read-through now, and maybe merge if everything else looks good!

Thanks again for your help.

DanilaFe

Thank you! I do have a few suggestions / design questions.

DanilaFe · 2023-01-21T00:45:33Z

+        const createEvent = state.getStateEvents("m.room.create", "");
+        const configEvent = state.getStateEvents(HIGHLIGHT_STATE_EVENT_TYPE, "")
+
+        return configEvent?.getContent()?.url || createEvent.getContent()[HIGHLIGHT_PAGE_KEY];


Suggested change

const createEvent = state.getStateEvents("m.room.create", "");

const configEvent = state.getStateEvents(HIGHLIGHT_STATE_EVENT_TYPE, "")

return configEvent?.getContent()?.url || createEvent.getContent()[HIGHLIGHT_PAGE_KEY];

const createEvent = state.getStateEvents("m.room.create", "");

const createData = createEvent.getContent()[HIGHLIGHT_PAGE_KEY];

const configEvent = state.getStateEvents(HIGHLIGHT_STATE_EVENT_TYPE, "")

return configEvent?.getContent()?.url || createData;

Though this makes me wonder: we get url out of the config event, but just return the createData from the creation event's content. Is the latter a string? Can the former be made a string (rather than an object?) too?

Reading the JS SDK, looks like an object is required. Oh well!

DanilaFe · 2023-01-21T00:52:33Z

+        (event.type === 'room-configured' ||
+            (event.type === 'room-membership' && event.membership === 'join'))
+        && state.tab === 'join') {
+        newState.tab = null


Joining a room from other tabs would close the join menu in this tab?

DanilaFe · 2023-01-21T00:55:22Z

            });
            this._sdkClient.on("Room.timeline", (event: sdk.MatrixEvent, room: sdk.Room, toStartOfTimeline: boolean, removed: boolean, data: {liveEvent: boolean}) => {
+                if (event.getType() === HIGHLIGHT_STATE_EVENT_TYPE) {
+                    this._emitRoom(room);


What if we're already in a room (we joined it) and someone else joins and tries to set the state event? Won't this double-count the events then?

DanilaFe · 2023-01-21T01:00:06Z

            this._loadRoom(message.roomId);
        }
    }
+    private async joinAndConfigureRoom(roomId: string, url: string) {


I'm not sure I like this approach (sorry 😞 ). Barging into a room and trying to set state events just seems... impolite? The use case for explicitly joining a room (rather than being invited, which is a case already handled, I hope) is some sort of public room that stores highlights. But that room would already be configured, then.

I suggest we split the two actions somehow:

Joining a room (this) -- you join an existing room and just assume it has a state event already set (or a create event). Then, the existing Room handler of the matrix client should take care of emitting the room, and all is well.

"Promoting" a room (new) -- take an existing room, that you're in and have permission to modify via state events -- and send the new state event to it, making it a room for your current URL.

Co-authored-by: Daniel <danila.fedorin@gmail.com>

Stvad added 3 commits January 2, 2023 15:59

upgrade matrix client minor version

947d978

Add "watch" script for faster iteration

9e475d4

Add webextension plugin to enable auto-reload in Chrome to improve it…

8123093

…eration speed. It also can provide better facilities for manifest manegement, but not using that now

Stvad mentioned this pull request Jan 4, 2023

Show menu if user tries to create a highlight without selecting a room first #23 #31

Merged

Stvad force-pushed the join-arbitrary-rooms branch from 7ef4b86 to 05bb8de Compare January 9, 2023 00:06

Stvad added 3 commits January 8, 2023 16:10

Readme dev instructions update

828e732

Join and configure arbitrary room. Iteration 1 (works but has issues) D…

3b699ab

…anilaFe#11

Fix chrome build issue (non-ascii symbols in minified version)

cbec6be

Stvad force-pushed the join-arbitrary-rooms branch from 05bb8de to cbec6be Compare January 9, 2023 00:12

DanilaFe reviewed Jan 21, 2023

View reviewed changes

update Import formatting

13f977b

Co-authored-by: Daniel <danila.fedorin@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to join an arbitrary room and use it as a target for higlights #11#28

Ability to join an arbitrary room and use it as a target for higlights #11#28
Stvad wants to merge 7 commits intoDanilaFe:masterfrom
Stvad:join-arbitrary-rooms

Stvad commented Jan 3, 2023

Uh oh!

Stvad commented Jan 3, 2023

Uh oh!

DanilaFe commented Jan 4, 2023

Uh oh!

Stvad commented Jan 4, 2023

Uh oh!

Stvad commented Jan 17, 2023

Uh oh!

DanilaFe commented Jan 21, 2023

Uh oh!

DanilaFe left a comment

Uh oh!

Uh oh!

DanilaFe Jan 21, 2023

Uh oh!

Uh oh!

DanilaFe Jan 21, 2023

Uh oh!

DanilaFe Jan 21, 2023

Uh oh!

DanilaFe Jan 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Stvad commented Jan 3, 2023

Uh oh!

Stvad commented Jan 3, 2023

Uh oh!

DanilaFe commented Jan 4, 2023

Uh oh!

Stvad commented Jan 4, 2023

Uh oh!

Stvad commented Jan 17, 2023

Uh oh!

DanilaFe commented Jan 21, 2023

Uh oh!

DanilaFe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanilaFe Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanilaFe Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

DanilaFe Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

DanilaFe Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants