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

Add a sidebar tab for documentation #1103

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

AustinMroz
Copy link
Collaborator

Much belated, but I wanted to find solutions sufficiently clean for core.

Adds a new sidebar tab for displaying documentation information on nodes. Nodes utilizing the existing tooltip functionality will have their descriptions parsed. While this pane is open, hovering over items on the active node will instead scroll to and briefly highlight the corresponding item in the documentation pane. Alternatively, a node can supply a list for its DESCRIPTION property which consists of

  • A short description which is used for node previews and title tooltips
  • A full html description
  • An optional dict to facilitate data transfer or provide additional configuration
    • render: A function called when the nodes documentation is displayed.
    • select: A function called when a node item has been hovered over.
      Note that both of these current options require a corresponding frontend extension to use. Few nodes will utilize this functionality, but it removes the need for ugly patching on those that do.

Further considerations

  • Having NodeTooltips import the callback directly from documentationSidebar causes documentationSidebar to load before extensionManager is set in app. The current implementation allows this tooltipCallback to be hooked by other extensions, but seems the cleanest implementation.
  • I quite dislike the question mark icon supplied by prime icons and implement an alternative with css. Mention was made of allowing icons other than those supplied by prime icons, but adding a generalized implementation here would be out of scope.

The formatting of ndoes using the existing standardized tooltips has
been improved.

Experiemental work for assisting nodes with display of more detailed
descriptions
Previously, description was a simple string, but supporting more complex
descriptions requires that new data be passed.

The type of a nodes description has been updated to be either a simple
string as before, or an array consisting of short description string, an
html string for the full description, and a placeholder dict for future
usage.

Definitions and usage points for description have been updated to
accommodate this change
Basic styling has been added to the display of documentation for nodes
using the existing tooltip system. This will need another pass to ensure
that style updates immediately when the light/dark toggle is hit instead
of requiring a change of node.

VHS specific namings have been replaced and the code for determining
what the mouse is hovering over has been removed. The existing tooltip
implementation is cleaner and will need to be integrated anyways so
tooltips are temporarily suppressed for the node actively being
displayed in the documentation sidebar.

Optional callbacks have been added for the initial sidebar display and a
user selecting a node element by hovering over it. While selection is
not yet implemented, this should cover any developer needs from more
involved collapsables to automated seeking to video timestamps.
Basic connecting for using the existing documentation hover code to
select an item from the active help pane.

Scrolling on selection will now properly perform the minium required
scroll to place the element on screen
Styling was moved to the sidebar element for better organization, but
this caused errors when the new menu was not in use.
Since the the css is now static the clutter of an added style element is
no longer needed
@huchenlei
Copy link
Member

I quite dislike the question mark icon supplied by prime icons and implement an alternative with css. Mention was made of allowing icons other than those supplied by prime icons, but adding a generalized implementation here would be out of scope.

We do have iconify setup now. You can find from a large set of iconify icons here: https://icon-sets.iconify.design/. Here is an example usage in this repo:

<template #icon>
<i-material-symbols:pan-tool-outline v-if="canvasStore.readOnly" />
<i-simple-line-icons:cursor v-else />
</template>

@huchenlei huchenlei self-requested a review October 4, 2024 19:08
@@ -714,3 +714,24 @@ audio.comfy-audio.empty-audio-widget {
.p-tree-node-content {
padding: var(--comfy-tree-explorer-item-padding) !important;
}

.doc-node {
Copy link
Member

Choose a reason for hiding this comment

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

Please move this to corresponding vue component.

Copy link
Member

Choose a reason for hiding this comment

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

Please write this sidebar tab as a vue component similar to other sidebar tab components here: https://github.com/Comfy-Org/ComfyUI_frontend/tree/39d68bcdc428751d0216e62fc6218b1555db1e46/src/components/sidebar/tabs

@@ -344,6 +344,11 @@ const zComfyComboOutput = z.array(z.any())
const zComfyOutputTypesSpec = z.array(
z.union([zComfyNodeDataType, zComfyComboOutput])
)
const zDescriptionSpec = z.union([
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide links to DESCRIPTION field used in custom nodes in this format?

@@ -101,7 +101,7 @@ export interface ComfyExtension {
export type ComfyObjectInfo = {
name: string
display_name?: string
description?: string
description?: [string | string, string | string, string, Record<string, any>]
Copy link
Member

Choose a reason for hiding this comment

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

export type DescriptionSpec = z.infer<zDescriptionSpec> in apiTypes.ts and use it here.

@@ -86,6 +86,11 @@ const onIdle = () => {
ctor.title_mode !== LiteGraph.NO_TITLE &&
canvas.graph_mouse[1] < node.pos[1] // If we are over a node, but not within the node then we are on its title
) {
if (comfyApp?.tooltipCallback?.(node, 'DESCRIPTION')) {
Copy link
Member

Choose a reason for hiding this comment

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

You can setup a pinia store to store/track the currently hovered node and node part instad of doing it via a callback. Setting up a pinia store will allow this state to be observable by multiple components if we need it later.

@huchenlei
Copy link
Member

We do have iconify setup now. You can find from a large set of iconify icons here: https://icon-sets.iconify.design/. Here is an example usage in this repo:

BTW I think I need to find a way support custom iconify icon. The iconify icon is lazy loaded during compile time so it won't be available for dynamic loaded extensions, but it should be necessary in this situation where in core we want to use iconify icon for sidebar tab icon.

@huchenlei
Copy link
Member

Related PR: #1169

By making LGraphCanvas shallowReactive, we can observe root object's state directly, and hopefully have the active widget/slot logic handled by litegraph to reduce the overhead of additional event listening.

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

Successfully merging this pull request may close these issues.

2 participants