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

More simple lightbox setup for SDK and compatible with custom History #749

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/domain/ViewModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@ import type {Clock} from "../platform/web/dom/Clock";
import type {ILogger} from "../logging/types";
import type {Navigation} from "./navigation/Navigation";
import type {URLRouter} from "./navigation/URLRouter";
import type {History} from "../platform/web/dom/History";

export type Options = {
platform: Platform
logger: ILogger
urlCreator: URLRouter
navigation: Navigation
history: History
emitChange?: (params: any) => void
}

Expand Down Expand Up @@ -142,4 +144,8 @@ export class ViewModel<O extends Options = Options> extends EventEmitter<{change
get navigation(): Navigation {
return this._options.navigation;
}

get history(): History {
return this._options.history;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #749 (comment) for why we're doing this now.

}
29 changes: 7 additions & 22 deletions src/domain/session/SessionViewModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {RoomViewModel} from "./room/RoomViewModel.js";
import {UnknownRoomViewModel} from "./room/UnknownRoomViewModel.js";
import {InviteViewModel} from "./room/InviteViewModel.js";
import {RoomBeingCreatedViewModel} from "./room/RoomBeingCreatedViewModel.js";
import {LightboxViewModel} from "./room/LightboxViewModel.js";
import {setupLightboxNavigation} from "./room/lightbox-navigation.js";
import {SessionStatusViewModel} from "./SessionStatusViewModel.js";
import {RoomGridViewModel} from "./RoomGridViewModel.js";
import {SettingsViewModel} from "./settings/SettingsViewModel.js";
Expand Down Expand Up @@ -81,12 +81,12 @@ export class SessionViewModel extends ViewModel {
}));
this._updateCreateRoom(createRoom.get());

const lightbox = this.navigation.observe("lightbox");
this.track(lightbox.subscribe(eventId => {
this._updateLightbox(eventId);
}));
this._updateLightbox(lightbox.get());

setupLightboxNavigation(this, 'lightboxViewModel', (eventId) => {
return {
room,
eventId,
};
});
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jun 8, 2022

Choose a reason for hiding this comment

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

Replacing all of the previous boilerplate to setup the lightbox with this composable function that external consumers can also use in the SDK.

This makes setting up the lightbox in your project that uses the SDK as simple as a setupLightboxNavigation(...) function call. See the usage in matrix-org/matrix-viewer#12


const rightpanel = this.navigation.observe("right-panel");
this.track(rightpanel.subscribe(() => this._updateRightPanel()));
Expand Down Expand Up @@ -267,21 +267,6 @@ export class SessionViewModel extends ViewModel {
this.emitChange("activeMiddleViewModel");
}

_updateLightbox(eventId) {
if (this._lightboxViewModel) {
this._lightboxViewModel = this.disposeTracked(this._lightboxViewModel);
}
if (eventId) {
const room = this._roomFromNavigation();
this._lightboxViewModel = this.track(new LightboxViewModel(this.childOptions({eventId, room})));
}
this.emitChange("lightboxViewModel");
}

get lightboxViewModel() {
return this._lightboxViewModel;
}

_roomFromNavigation() {
const roomId = this.navigation.path.get("room")?.value;
const room = this._client.session.rooms.get(roomId);
Expand Down
20 changes: 12 additions & 8 deletions src/domain/session/room/LightboxViewModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,25 @@ import {ViewModel} from "../../ViewModel";
export class LightboxViewModel extends ViewModel {
constructor(options) {
super(options);
this._eventId = options.eventId;
this._eventEntry = options.eventEntry;
this._eventId = options.eventId || options.eventEntry.id;
this._unencryptedImageUrl = null;
this._decryptedImage = null;
this._closeUrl = this.urlCreator.urlUntilSegment("room");
this._eventEntry = null;
this._date = null;
this._subscribeToEvent(options.room, options.eventId);
}

_subscribeToEvent(room, eventId) {
const eventObservable = room.observeEvent(eventId);
this.track(eventObservable.subscribe(eventEntry => {
this._loadEvent(room, eventEntry);
}));
this._loadEvent(room, eventObservable.get());
let event = this._eventEntry;
if (!this._eventEntry) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the option of passing in eventEntry directly since we have access to all of the events already in matrix-public-archive and we don't need or want to load more events.

Previously, this only allowed passing eventId which loaded in the full event.

MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
const eventObservable = room.observeEvent(eventId);
this.track(eventObservable.subscribe(eventEntry => {
this._loadEvent(room, eventEntry);
}));
event = eventObservable.get();
}
this._loadEvent(room, event);
}

async _loadEvent(room, eventEntry) {
Expand Down Expand Up @@ -92,6 +96,6 @@ export class LightboxViewModel extends ViewModel {
}

close() {
this.platform.history.pushUrl(this.closeUrl);
this.history.pushUrl(this.closeUrl);
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jun 8, 2022

Choose a reason for hiding this comment

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

Since we're using a custom History implementation in matrix-org/matrix-viewer#12 to get hashes relative to the room, we need to use that custom History over the default one from platform.

The only other place we use platform.history is in src/platform/web/main.js#L38

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to inject your custom history through platform.history.

}
}
69 changes: 69 additions & 0 deletions src/domain/session/room/lightbox-navigation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
Copyright 2022 Bruno Windels <[email protected]>
Copyright 2022 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import {LightboxViewModel} from "./LightboxViewModel.js";

// Store the `LightboxViewModel` under a symbol so no one else can tamper with
// it. This acts like a private field on the class since no one else has the
// symbol to look it up.
let lightboxViewModelSymbol = Symbol('lightboxViewModel');

/**
* Destroys and creates a new the `LightboxViewModel` depending if
* `lightboxChildOptions.eventEntry` or `lightboxChildOptions.eventId` are
* provided.
*/
function updateLightboxViewModel(vm, fieldName, lightboxChildOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I think this kind of dynamic mixin programming makes the code as a whole a lot less readable. Also, this will prevent us to make the track and disposeTracked method protected once we're fully converted to typescript.

I can see a case for having a generic helper method on ViewModel to setup a child view model based on a navigation observable, but I'd do it differently, something like this:

class ViewModel {
	bindChildToNavigationSegment(segmentName, setter, getter, vmMapper, publicPropName) {
		const update = value => {
			if (getter()) {
	            setter(this.disposeTracked(getter()));
	        }
	        if (value) {
	            setter(this.track(vmMapper(value)));
	        }
	        this.emitChange(publicPropName);
		}

		const observable = this.navigation.observe(segmentName);
        this.track(observable.subscribe(update));
        update(observable.get());
	}
}

Copy link
Contributor Author

@MadLittleMods MadLittleMods Oct 19, 2022

Choose a reason for hiding this comment

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

To make it make sense it my head, here would be the usage for lightbox (adapted to using object parameters since there is so many flying around here):

this.bindChildToNavigationSegment({
  segmentName: 'lightbox',
  setter: (newValue) => {
    this._lightboxViewModel = newValue;
  },
  getter: () = {
    return this._lightboxViewModel;
  },
  vmMapper: (eventId) = {
    return new LightboxViewModel(this.childOptions({eventId, room}));
  },
  publicPropName: 'lightboxViewModel'
});

bindChildToNavigationSegment does make a lot of sense to capture this abstraction because it's so common 👍

The usage seems like too much boilerplate for what a SDK consumer cares about though.


I think we could boil it down to:

this.bindChildToNavigationSegment({
  segmentName: 'lightbox',
  vmMapper: (eventId) = {
    return new LightboxViewModel(this.childOptions({eventId, room}));
  },
  publicPropName: 'lightboxViewModel'
});
class ViewModel {
  bindChildToNavigationSegment({ segmentName, vmMapper, publicPropName }) {
        // Store the `ViewModel` under a symbol so no one else can tamper with
        // it. This acts like a private field on the class since no one else has the
        // symbol to look it up.
      let viewModelSymbol = Symbol(publicPropName);

      // On the given `vm`, create a getter at `publicPropName` that the
        // `ViewModel` is exposed at for usage in the view.
        Object.defineProperty(vm, publicPropName, {
            get: function() {
                return vm[viewModelSymbol];
            }
        });
        
    const update = value => {
      if (vm[lightboxViewModelSymbol]) {
              vm[lightboxViewModelSymbol] = this.disposeTracked(getter());
          }
          if (value) {
              vm[lightboxViewModelSymbol] = this.track(vmMapper(value));
          }
          this.emitChange(publicPropName);
    }

    const observable = this.navigation.observe(segmentName);
        this.track(observable.subscribe(update));
        update(observable.get());
  }
}

If some use case needs getter/setter we could have those as extra options and store it for them by default as above ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bwindels Does the boiled down version look reasonable?

// Remove any existing `LightboxViewModel` before we assemble the new one below
if (vm[lightboxViewModelSymbol]) {
vm[lightboxViewModelSymbol] = vm.disposeTracked(vm[lightboxViewModelSymbol]);
// Let the `LightboxView` know that the `LightboxViewModel` has changed
vm.emitChange(fieldName);
}
// Create the new `LightboxViewModel` if the `eventEntry` exists directly or
// `eventId` which we can load from the store
if (lightboxChildOptions.eventId || lightboxChildOptions.eventEntry) {
vm[lightboxViewModelSymbol] = vm.track(new LightboxViewModel(vm.childOptions(lightboxChildOptions)));
// Let the `LightboxView` know that the `LightboxViewModel` has changed
vm.emitChange(fieldName);
}
}

/**
* Handles updating the `LightboxViewModel` whenever the page URL changes and
* emits changes which the `LightboxView` will use to re-render. This is a
* composable piece of logic to call in an existing `ViewModel`'s constructor.
*/
export function setupLightboxNavigation(vm, fieldName = 'lightboxViewModel', lightboxChildOptionsFunction) {
// On the given `vm`, create a getter at `fieldName` that the
// `LightboxViewModel` is exposed at for usage in the view.
Object.defineProperty(vm, fieldName, {
get: function() {
return vm[lightboxViewModelSymbol];
}
});

// Whenever the page navigates somewhere, keep the `lightboxViewModel` up to date
const lightbox = vm.navigation.observe("lightbox");
vm.track(lightbox.subscribe(eventId => {
updateLightboxViewModel(vm, fieldName, lightboxChildOptionsFunction(eventId));
}));
// Also handle the case where the URL already includes `/lightbox/$eventId` (like
// from page-load)
const initialLightBoxEventId = lightbox.get();
updateLightboxViewModel(vm, fieldName, lightboxChildOptionsFunction(initialLightBoxEventId));
}
3 changes: 3 additions & 0 deletions src/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ export {SessionViewModel} from "./domain/session/SessionViewModel.js";
export {SessionView} from "./platform/web/ui/session/SessionView.js";
export {RoomViewModel} from "./domain/session/room/RoomViewModel.js";
export {RoomView} from "./platform/web/ui/session/room/RoomView.js";
export {LightboxView} from "./platform/web/ui/session/room/LightboxView.js";
export {setupLightboxNavigation} from "./domain/session/room/lightbox-navigation.js";
export {TimelineViewModel} from "./domain/session/room/timeline/TimelineViewModel.js";
export {tileClassForEntry} from "./domain/session/room/timeline/tiles/index";
export type {TimelineEntry, TileClassForEntryFn, Options, TileConstructor} from "./domain/session/room/timeline/tiles/index";
Expand Down Expand Up @@ -62,6 +64,7 @@ export {TextMessageView} from "./platform/web/ui/session/room/timeline/TextMessa
export {VideoView} from "./platform/web/ui/session/room/timeline/VideoView.js";

export {Navigation} from "./domain/navigation/Navigation.js";
export {History} from "./platform/web/dom/History.js";
export {ComposerViewModel} from "./domain/session/room/ComposerViewModel.js";
export {MessageComposer} from "./platform/web/ui/session/room/MessageComposer.js";
export {TemplateView} from "./platform/web/ui/general/TemplateView";
Expand Down
4 changes: 2 additions & 2 deletions src/platform/web/dom/History.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ export class History extends BaseObservableValue {
But for SSO, we need to handle <root>/?loginToken=<TOKEN>
Handle that as a special case for now.
*/
if (document.location.search.includes("loginToken")) {
if (document?.location?.search.includes("loginToken")) {
return document.location.search;
}
return document.location.hash;
return document?.location?.hash;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to allow this function to run when server-side rendered in linkedom. It's ok for this function to return undefined. See shared/lib/archive-history.js for how this is integrated.

If this isn't desired, I can always wrap super.get() in my custom History instance and default that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be better if you add your custom logic in your custom instance. It's going to be hard to not have regressions for some arcane DOM impl we never test for.

}

/** does not emit */
Expand Down
30 changes: 30 additions & 0 deletions src/platform/web/ui/general/Link.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
Copyright 2020 Bruno Windels <[email protected]>

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import {TemplateView} from "./general/TemplateView";

export class LinkView extends TemplateView {
render(t, vm) {
return t.a({
...vm,
onClick: (e) => {
// Allow the `urlRouter` to cancel the URL navigation and any upstream
// consumer that added their own `onClick` handler.
return vm.urlCreator.linkClick(this, e) || vm.onClick?.(e);
}
});
}
}