Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Reuse empty string & space string logic for event types in devtools #9218

Merged
merged 4 commits into from
Aug 25, 2022
Merged

Reuse empty string & space string logic for event types in devtools #9218

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edd7354
Reuse empty string & space string logic for event types in devtools
t3chguy Aug 24, 2022
5e51709
Fix edge case behaviours
t3chguy Aug 24, 2022
78efcfc
Make tsc quality higher
t3chguy Aug 24, 2022
bfd8eb0
Make remainder of tsc happy
t3chguy Aug 24, 2022
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
75 changes: 37 additions & 38 deletions src/components/views/dialogs/devtools/RoomState.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,52 +41,61 @@ export const StateEventEditor = ({ mxEvent, onBack }: IEditorProps) => {
return <EventEditor fieldDefs={fields} defaultContent={defaultContent} onSend={onSend} onBack={onBack} />;
};

interface StateEventButtonProps {
label: string;
onClick(): void;
}

const StateEventButton = ({ label, onClick }: StateEventButtonProps) => {
const trimmed = label.trim();
Copy link
Contributor

@jaller94 jaller94 Aug 24, 2022

Choose a reason for hiding this comment

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

.trim() is for all whitespaces.

I think we're opening up a box that the spec should have sealed.

Whitespace in this context is all the whitespace characters (space, tab, no-break space, etc.) and all the line terminator characters (LF, CR, etc.).

Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/Trim


return <button
className={classNames("mx_DevTools_button", {
mx_DevTools_RoomStateExplorer_button_hasSpaces: trimmed.length !== label.length,
mx_DevTools_RoomStateExplorer_button_emptyString: !trimmed,
})}
onClick={onClick}
>
{ trimmed ? label : _t("<%(count)s spaces>", { count: label.length }) }
Copy link
Contributor

@jaller94 jaller94 Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As indicated above, this may also be tabs or other white spaces.
" " and "\t" would have the name "1 space".

Copy link
Member Author

@t3chguy t3chguy Aug 24, 2022

Choose a reason for hiding this comment

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

Sure, feel free to open a new issue for that, it isn't a new issue

Copy link
Contributor

@jaller94 jaller94 Aug 24, 2022

Choose a reason for hiding this comment

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

When I first saw the screenshot I thought of Matrix Spaces.
Only wen I read the code, I realized that this name is meant to represent invisible characters.

Copy link
Member Author

@t3chguy t3chguy Aug 24, 2022

Choose a reason for hiding this comment

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

This isn't a new issue though

</button>;
};

interface IEventTypeProps extends Pick<IDevtoolsProps, "onBack"> {
eventType: string;
}

const RoomStateExplorerEventType = ({ eventType, onBack }: IEventTypeProps) => {
const context = useContext(DevtoolsContext);
const [query, setQuery] = useState("");
const [event, setEvent] = useState<MatrixEvent>(null);
const [event, setEvent] = useState<MatrixEvent | null>(null);

const events = context.room.currentState.events.get(eventType);
const events = context.room.currentState.events.get(eventType)!;

useEffect(() => {
if (events.size === 1 && events.has("")) {
setEvent(events.get(""));
setEvent(events.get("")!);
} else {
setEvent(null);
}
}, [events]);

if (event) {
const onBack = () => {
setEvent(null);
const _onBack = () => {
Copy link
Contributor

@jaller94 jaller94 Aug 24, 2022

Choose a reason for hiding this comment

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

I know this inside an if block and probably only rendered once on the screen,

still I'd like to point out that ideally this would be wrapped in an useCallBack(…, [events, onBack]).

Copy link
Member Author

@t3chguy t3chguy Aug 24, 2022

Choose a reason for hiding this comment

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

You can't use useCallback inside an if statement.
https://reactjs.org/docs/hooks-rules.html#:~:text=Don't%20call%20Hooks%20inside,each%20time%20a%20component%20renders.

if (events?.size === 1 && events.has("")) {
onBack();
} else {
setEvent(null);
}
};
return <EventViewer mxEvent={event} onBack={onBack} Editor={StateEventEditor} />;
return <EventViewer mxEvent={event} onBack={_onBack} Editor={StateEventEditor} />;
}

return <BaseTool onBack={onBack}>
<FilteredList query={query} onChange={setQuery}>
{
Array.from(events.entries()).map(([stateKey, ev]) => {
const trimmed = stateKey.trim();
const onClick = () => {
setEvent(ev);
};

return <button
className={classNames("mx_DevTools_button", {
mx_DevTools_RoomStateExplorer_button_hasSpaces: trimmed.length !== stateKey.length,
mx_DevTools_RoomStateExplorer_button_emptyString: !trimmed,
})}
key={stateKey}
onClick={onClick}
>
{ trimmed ? stateKey : _t("<%(count)s spaces>", { count: stateKey.length }) }
</button>;
})
Array.from(events.entries()).map(([stateKey, ev]) => (
<StateEventButton key={stateKey} label={stateKey} onClick={() => setEvent(ev)} />
Copy link
Contributor

@jaller94 jaller94 Aug 24, 2022

Choose a reason for hiding this comment

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

If this list of events is long, performance could be improved by using useCallback.

Copy link
Member Author

@t3chguy t3chguy Aug 24, 2022

Choose a reason for hiding this comment

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

You can't use useCallback in a loop - https://reactjs.org/docs/hooks-rules.html#:~:text=Don't%20call%20Hooks%20inside,each%20time%20a%20component%20renders.

Copy link
Contributor

@jaller94 jaller94 Aug 24, 2022

Choose a reason for hiding this comment

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

You can define a new functional component outside this functional component. :)

))
}
</FilteredList>
</BaseTool>;
Expand All @@ -95,11 +104,11 @@ const RoomStateExplorerEventType = ({ eventType, onBack }: IEventTypeProps) => {
export const RoomStateExplorer = ({ onBack, setTool }: IDevtoolsProps) => {
const context = useContext(DevtoolsContext);
const [query, setQuery] = useState("");
const [eventType, setEventType] = useState<string>(null);
const [eventType, setEventType] = useState<string | null>(null);

const events = context.room.currentState.events;

if (eventType) {
if (eventType !== null) {
const onBack = () => {
setEventType(null);
};
Expand All @@ -113,19 +122,9 @@ export const RoomStateExplorer = ({ onBack, setTool }: IDevtoolsProps) => {
return <BaseTool onBack={onBack} actionLabel={_t("Send custom state event")} onAction={onAction}>
<FilteredList query={query} onChange={setQuery}>
{
Array.from(events.keys()).map((eventType) => {
const onClick = () => {
setEventType(eventType);
};

return <button
className="mx_DevTools_button"
key={eventType}
onClick={onClick}
>
{ eventType }
</button>;
})
Array.from(events.keys()).map((eventType) => (
<StateEventButton key={eventType} label={eventType} onClick={() => setEventType(eventType)} />
Copy link
Contributor

@jaller94 jaller94 Aug 24, 2022

Choose a reason for hiding this comment

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

If this list of events is long, performance could be improved by using useCallback.

Copy link
Member Author

@t3chguy t3chguy Aug 24, 2022

Choose a reason for hiding this comment

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

You can't use useCallback in a loop - https://reactjs.org/docs/hooks-rules.html#:~:text=Don't%20call%20Hooks%20inside,each%20time%20a%20component%20renders.

))
}
</FilteredList>
</BaseTool>;
Expand Down