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 Settings #14

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

Add Settings #14

wants to merge 12 commits into from

Conversation

sn0w12
Copy link
Contributor

@sn0w12 sn0w12 commented Sep 6, 2024

Add several settings to help users customize their experience, I tried to make it simple to add new settings. Changing a setting triggers a custom event so that we don't have to check for changes every time we want to use a setting, the event is also triggered on startup to initialize the values.

Settings

  1. Keep Minimap In Bounds
    • Ensures that the minimap remains within the window boundaries.
    • Default: True
  2. Restrict Minimap to Graph
    • Ensures the minimap remains within the graph area and doesn't overlap any UI components, this also applies when dragging the minimap around. This is only tested on the new UI, I am unsure if it would work on the legacy UI.
    • Default: True
  3. Snap To
    • Allows users to control where the minimap snaps on the screen. This currently has these options:
      • None
      • Relative
      • Top Right, Left
      • Bottom Righ, Left
    • Default: None

Settings

@sn0w12 sn0w12 mentioned this pull request Sep 6, 2024
@OliverCrosby
Copy link
Owner

I like these changes but I'm having some issues with it. I'm on ComfyUI v0.2.2 and tried it in both chrome and firefox.

  1. it seems to be slowing down the UI, especially on firefox.
  2. Is not consistently remembering the 'Snap-to relative' setting when page is refreshed. Often it seems to revert back to a previous setting upon refresh.
  3. sometimes when I refresh the page, the minimap is not visible because it's left or right of the browser window.
  4. 'Restrict Minimap to Graph' (which I think should be called something like 'keep minimap under UI, or something): pushes the UI and graph to the left and locks the minimap to the right side of the screne, where it can move up and down. I don't think this is intended behaviour, screenshot:
    image

@sn0w12
Copy link
Contributor Author

sn0w12 commented Sep 13, 2024

I'll look into it, the Restrict Minimap to GraphI believe is because I am using the beta menu and forgot it was still in beta, it doesn't really have a use outside of it so i'll make it default to false and check if the user has the beta menu enabled.

The relative positions isn't stored anywhere which is a bit of an oversight but we can just store it in localstorage and use that on load.

The performance I don't really know but when I look at it now it definitely is lower than usual. I'll try to see if I can find the issue.

Edit: Actually, the performance is completely fine for me, i checked on firefox and chrome again and it was running even better on firefox than chrome. So I don't really know if I can look into it that well.

@sn0w12
Copy link
Contributor Author

sn0w12 commented Sep 13, 2024

  1. Is not consistently remembering the 'Snap-to relative' setting when page is refreshed. Often it seems to revert back to a previous setting upon refresh.

Every time we resize the screen or stop dragging the minimap we save the relative position to localstorage and then load it on load.

  1. sometimes when I refresh the page, the minimap is not visible because it's left or right of the browser window.
  2. 'Restrict Minimap to Graph' pushes the UI and graph to the left and locks the minimap to the right side of the screne, where it can move up and down. I don't think this is intended behaviour

Both of these issues came from the Restrict Minimap to Graph since it only works with the beta menu. I changed it so it is a beta setting and mentioned that it only works with the beta menu in the tooltip. The setting doesn't do anything if the beta menu is disabled anymore. And although I agree that "Restrict Minimap to Graph" isn't exactly the greatest name I don't know if "keep minimap under UI" is right either since the setting forces the minimap to be inside the content area not exactly below the UI. But I can't really think of anything better myself so that's really up to you.

Settings

@sn0w12
Copy link
Contributor Author

sn0w12 commented Sep 16, 2024

Since we can't know if the beta menu setting is changed easily, now we just check if the padding is larger or equal to the size of comfy and if it is then return 0 padding. This also stops the issue as far as I can tell while also staying responsive if you decide to change the menu. Also just a nice safeguard to have if something goes wrong with the padding for whatever reason.

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