matrix-highlight icon indicating copy to clipboard operation
matrix-highlight copied to clipboard

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

Open Stvad opened this issue 2 years ago • 5 comments

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) #11

Stvad avatar Jan 03 '23 00:01 Stvad

"fixes" #27 too

Stvad avatar Jan 03 '23 22:01 Stvad

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?

DanilaFe avatar Jan 04 '23 02:01 DanilaFe

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)

Stvad avatar Jan 04 '23 02:01 Stvad

Hi, checking in on this! :)

Stvad avatar Jan 17 '23 03:01 Stvad

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 avatar Jan 21 '23 00:01 DanilaFe