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

Allow the DisabledComposerView to have a HTML templated description #864

Closed
wants to merge 1 commit into from

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Aug 31, 2022

Allow the DisabledComposerView to have a HTML templated description. Changes used in matrix-org/matrix-viewer#54 so we can add a link.

description: [
      `You're viewing an archive of events from ${dateString}. Use a `,
      tag.a(
        {
          href: matrixPublicArchiveURLCreator.permalinkForRoomId(roomData.id),
          rel: 'noopener',
          target: '_blank',
        },
        ['Matrix client']
      ),
      ` to start chatting in this room.`,
    ],

The previous behavior is still possible by providing description: (vm) => vm.description.

Dev notes

  • LowerPowerLevelViewModel
  • ArchivedViewModel

@MadLittleMods MadLittleMods added feature New feature or request sdk labels Aug 31, 2022
@MadLittleMods MadLittleMods marked this pull request as ready for review August 31, 2022 03:27
@MadLittleMods MadLittleMods marked this pull request as draft August 31, 2022 03:40
@MadLittleMods MadLittleMods marked this pull request as ready for review August 31, 2022 04:06
Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

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

I'm very surprised if this actually allows you to return DOM nodes from the VM property (not something we typically do in MVVM but I can see why you want to take that shortcut here) rather than a string, as the change you made should actually add a text binding which coerces the result of the binding to a string and updates the DOM by setting node.textContent... did you actually see this working?

Ideally you would be able to inject an alternative view for DisabledComposerView but there is no good way to do this currently without breaking tree-shaking during bundling (which you don't care about as you're on the server, but for browser builds we do actually want). Need to think of a better solution...

@MadLittleMods
Copy link
Contributor Author

@bwindels The current method works and what drives this UI in the archive:

Need to think of a better solution...

It can't just be a string because it has a link in the middle of it. Any way forward will work though 🙂

@bwindels
Copy link
Contributor

Yeah, I know just a string is not what you need, I was just trying to make sense of how it can work. But actually, I just realized I was reading the diff wrong 🤦 You are removing the text binding, rather than adding it. In that case, yes, I can see how that would allow you to return DOM nodes from the view model.

I'd be fine to take this shortcut for now, but the problem with your change is that updates from the ArchivedViewModel won't be reflected in the UI anymore (also see RoomViewModel._onRoomChange)...

As I said, I need to think of a better solution ...

@bwindels
Copy link
Contributor

Actually, I think the proper thing to do here will be to use the RoomHeaderView and TimelineView separately rather than the RoomView, and take care of the rest of the layout in your own code.

@MadLittleMods
Copy link
Contributor Author

MadLittleMods commented Oct 19, 2022

Actually, I think the proper thing to do here will be to use the RoomHeaderView and TimelineView separately rather than the RoomView, and take care of the rest of the layout in your own code.

This seems like a cop-out to me. Although this seems fine today because RoomView is such a lightweight wrapper, we end up inventing the same layout and lose any future updates (whether that be features or bugfixes) without maintenance burden to copy and adapt. The goal is to update the Hydrogen SDK and be up to date instead of the Matrix Public Archive falling behind forever after I stop focusing on it like every other peripheral project (view.matrix.org, matrix.to, matrix.org, spec.matrix.org). Chatterbox is lucky in this regard. Its SDK usage (and was developed alongside) is so close to Hydrogen that everything just fits already (AFAIK) and I want to get us to the same stage.

DisabledComposerView exactly describes the functionality that we need in the archive as well. It even pulls from something called ArchivedViewModel. These are just names but the overlap is uncanny. I'm sure there are scenarios where you would want HTML in the DisabledComposerView used in Hydrogen itself as well (like linking some help documentation or a "Join" button after you've left) so we just need to use that same mechanism (whatever it turns out to be).

In some cases, whole components might need to be swapped but maybe we need slots for that (see how I replaced the RoomHeaderView in RoomView).

To me, it seems like we're just running into a composability problem. Things are way too tightly coupled to random Hydrogen things with hard to penetrate abstractions. Right now, we're running into some teething problems trying to adapt the one and only Hydrogen use case to something slightly different.

It still seems crazy that I can't just provide a room meta data object and a list of events to a RoomView (or the RoomViewModel) and have it display a room with no side-effects. I still feel like mocking the HTTP layer isn't necessary although is better than fighting current abstractions. The abstractions can be better though.

I know you have apprehension and believe the use cases are more separate than how I look at it so some compromise is probably the correct solution to some of these things.

@bwindels
Copy link
Contributor

bwindels commented Oct 20, 2022

I hear you, but our understanding of what hydrogen-view-sdk is for seems to be misaligned.

To me, it's a library to build or embed an interactive matrix chat client. You can either build your own UI on top of src/matrix, or you settle for (a subset of) the UI similar to Hydrogen and also reuse the view.

From your comment above, you seem to view it as a collection of chat-related utilities from which you can grab any component in isolation and expect a logical and extensible interface close to your needs.

Given your use case, I can see why you would want that, but I think that we just don't have the manpower to provide that and it might also collide with some optimizations.

The biggest reason the abstractions are sometimes not as clean as you would like is that Hydrogen relies heavily on storage to be fast. It stores data in denormalized fashion as it comes in through sync. That implementation detail bleeds into most classes in src/matrix, which is why I don't want to make the constructor for those classes public (e.g. export them from lib.ts).

Having said that, I'm sure things can be improved, and if you have any concrete suggestions where abstractions could be cleaned up, I'd be happy to hear them.

I continue to suspect that using TimelineView rather than RoomView would make more sense for your use case. In the future, RoomView will indeed evolve, but possibly in ways that don't simplify things for your use case. For example, we'll soon add support to show video calls in RoomView. We might add widget support some day. All things I imagine you have no need for. Because ... what you want to show is the timeline. The fact that you need to replace the room header already shows your needs are not exactly the same as a chat client.
IMO, having to implement the layout between the timeline, header and banner yourself is outweighed by your current need to mock the room view model completely, in ways that are bound to break in the future as the room UI allows more and more interactions not related to the timeline.

I'd be open however to your idea of allowing to inject subview constructors into a view (slots), but worry a bit that we'll then have a lot of requests to slightly change the view models and parent views to accomodate the need of alternative views. And all of a sudden we have a lot of behaviour that is not used in the main app and hence remains untested and can easily regress. Perhaps once we have typescript interfaces for all view models, it would be easier to draw a line here.

There is also the fact that this approach, doing injection at runtime, prevents proper tree-shaking of code that is not used. But I can't think of a good way to do it at compile-time while providing the SDK as an npm package, so perhaps the runtime approach is better than nothing.

Wrt to the mock-http-storage approach, if it helps, we could put a createClientFromEvents(roomId, events): Promise<Client> utility function in a separate library if you're worried about the maintenance of that approach. That would keep the mock storage and http mock internally to the utility.

@MadLittleMods
Copy link
Contributor Author

MadLittleMods commented Nov 2, 2022

I continue to suspect that using TimelineView rather than RoomView would make more sense for your use case. In the future, RoomView will indeed evolve, but possibly in ways that don't simplify things for your use case.

In any case, I've refactored things to use TimelineView for now, matrix-org/matrix-viewer#117

For example, we'll soon add support to show video calls in RoomView. We might add widget support some day. All things I imagine you have no need for.

Video call rooms don't have much historical value without playback but there could be many more room types that would. Also depends on the type of widgets but they could have historical context as well. Like if polls was an inline widget as originally proposed, it would be nice to see the vote summary. Or a widget that shows the review queue count in the UI, it would be nice to see the count at the time (granted the widget needs to be smarter than grab the latest state for this kind of thing).

We want many of the fancy features of the room/timeline today, lightbox, context menu options including dialog/modals like "View source", stick to bottom/top, member list and inspecting a single member, probably some spaces hierarchy exploration, threading.

What about threads? Will threads be implemented as part of RoomView with multiple timelines or as part of TimelineView, etc. We definitely want to show threads in the archive without implementing it ourselves (and the always evolving set of features into the future).

The fact that you need to replace the room header already shows your needs are not exactly the same as a chat client.

To be clear, I don't like this aspect of having our own custom RoomHeaderView. For example, Hydrogen doesn't support the room topic/description. While a naive text implementation will be easy to copy from Hydrogen if it happens (or add ourselves), other features probably won't be. For example, this will become a lot more involved when Hydrogen also supports clicking the room topic to view it in its full formatted glory or whatever approach to solve that.

Ideally, we would be using the same RoomHeaderView with some options conditionally hidden and composed in. It's hard to argue for this at the moment because it's way easier just to have our custom version with how simple everything is here.

IMO, having to implement the layout between the timeline, header and banner yourself is outweighed by your current need to mock the room view model completely, in ways that are bound to break in the future as the room UI allows more and more interactions not related to the timeline.

This is a great point if the simple grid layout was all that is necessary 👍

Wrt to the mock-http-storage approach, if it helps, we could put a createClientFromEvents(roomId, events): Promise<Client> utility function in a separate library if you're worried about the maintenance of that approach. That would keep the mock storage and http mock internally to the utility.

Maybe. I don't really have problem with doing the fake HTTP part of it especially since the use-case can morph. The rest of the Hydrogen platform boilerplate is the part that downstream consumers don't care about. And the wobbly part is that Hydrogen can call the HTTP API's differently than how you expect later down the line.

But as an alternative, it feels more like I should be able to do createRoomWithEvents('!foo:bar', events): Promise<Room>

Pseudo code
const room = await createRoomWithEvents('!foo:bar', events);
const timeline = await room.openTimeline();

const timelineVM = new TimelineViewModel({
  timeline,
  roomVM: { room }
});
const view = new TimelineView(timelineVM, viewClassForTile);
view.mount();

const app = document.querySelector('#app');
app.appendChild(view.root());


Closing this PR as it's no longer necessary but would love to continue this conversation to make things more sane.

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

Successfully merging this pull request may close these issues.

2 participants