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

Image Feed: UX Overhauling #63

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

Conversation

birdddev
Copy link
Contributor

@birdddev birdddev commented Sep 3, 2023

  • Makes the panel resizable using a bar on the open side
  • Slight styling changes
  • Removed column amount/image feed size options
  • Added image size option
  • Grid should auto-adjust based on the panel size and desired image size
  • Holding Ctrl while resizing will snap the panel to the current image size * step
  • Changed the "Resize Feed" setting text to a Cog emoji
  • Use the .div for image items instead of .img to adjust brightness (The image could be smaller than parts of the button due to differing aspect ratios, but the button itself is always the desired image size. 1:1)
  • Moved css variables to the new image-feed-root
  • Always show the inner feed box
out.mp4

I ended up having free time unexpectedly, so I went ahead and tried to work on some of those suggestions I had.

Doing a draft for now, since I would like feedback on the current changes from another user's perspective, and it's slightly broken in a few ways at the moment. Also pretty messy.
The "right" location settings menu is broken because it's attached to the panel, so width changes from adjusting image size makes it do bad things.


Definitely want

  • Hold shift while changing panel's width to also adjust image size. (This plus the Ctrl + Drag should make it have feature parity with the previous setup, but with a more intuitive way of adjusting.)
  • Rework the settings box to be a "pop-up" that isn't parented to the feed. (Is there a proper way to do this already with something from comfy/litegraph? I know it has pop-ups, but I don't know how adjustable they are, or how I would go about using them.)
  • Fix flickering cursor when resizing too fast.
  • Make empty feed not take up more space than header buttons.
  • Update Readme with new features.
  • Fix vertical locations not expanding feed height fully.

Nice to have

  • Adjust the panel's width after changing locations and on load so it doesn't snap outward. (Or change it so the images shrink like I wanted to do and allow for the panel to be dragged smaller than image size.)
  • An option to show live previews in the image feed where the next image would be instead of on the prompt thing.
  • Have a little area near the settings/clear/close buttons, that when dragging, shows a preview to snap the panel to a different location. On release, changes the location to it. Just moved to the setting popup instead.
  • A switch for descending/ascending images in the "header bar" or in the panel's settings. (These last 2 should allow you to remove those options from the ComfyUI settings to reduce clutter.)
  • Separator bar per prompt for each collection of images (if a prompt puts multiple images in the feed, then separate from next queue prompt? Make optional probably, opt-in even, as it may be ugly?)

- Makes the panel resizable using a bar on the open side
- Slight styling changes
- Removed column/image feed size options
- Added image size option
- Grid should auto-adjust based on the size and desired image size
- Holding Ctrl while resizing will snap to the current image size * step
- Changed the "Resize Feed" setting text to a Cog emoji
- Use the .div for image containers instead of .img to adjust brightness
- Moved css variables to the new image-feed-root
- Always show the inner feed box
Fixes the flickering for when the mouse is too fast for the handle
@pythongosssss
Copy link
Owner

pythongosssss commented Sep 5, 2023

Looks really nice so far!

(Is there a proper way to do this already with something from comfy/litegraph)

Not really anything nice, there is the litegraph ContextMenu but it isnt overly useful for this scenario
My only comment with the current state is that when the feed is empty, it shouldnt take up more space than the header buttons

@birdddev
Copy link
Contributor Author

birdddev commented Sep 7, 2023

Oh, yeah just having min width be the size of the header buttons is super good. I assume vertically I would let the empty feed be able to get cut off, but the header needs to be always visible somehow.

Not really anything nice...

Shame, I can make my own implementation for now (I don't really like how the popups in comfy/litegraph work anyways, as clicking outside of them don't cancel them). I Would've liked to use something that exists and improve it upstream, but I can use this to try stuff and then get it upstreamed later.

Where would you want me to put the widget implementation? Not the image feed's settings menu, but the re-usable popup widget itself, or should I do a separate pull request for that?

@pythongosssss
Copy link
Owner

You can either just put it in the image feed file for now and I can move it out if it gets reused, or into the common folder

See image feed for an example of usage.
Popup shouldn't go off the bottom screen now
@birdddev
Copy link
Contributor Author

birdddev commented Sep 9, 2023

Here is how the popup looks.
image
image
image
It will stay open until you click somewhere outside the popup, or if you hover over the popup "window" and then mouseleave. There's also an option for triggering the window on hover of the attached element, or on click.
It should try to always stay inside the window.

birdddev and others added 6 commits September 13, 2023 21:57
Switch from width to min-width/height for the panel size
Makes it so when the feed is empty, the width/height is more than any
content inside
So the header buttons should be the new minimum width for the panel
When there are images, the panel will expand to desired image size
Closes when mouse distance is too far
Adds an option to set the max distance, defaulting to 50
Makes the container more opaque
Fixes outside of window logic
Update image feed stuff
@birdddev birdddev marked this pull request as ready for review September 14, 2023 12:17
Every side now makes the image size bigger when pulling the panel out
@pythongosssss
Copy link
Owner

Looking really nice! a couple of comments from testing:

  • You can't resize the panel so images are overflowed with the panel on the top/bottom, i.e. i can't make the height smaller than this:
    image

  • When the panel is in top/bottom mode, you can add .pysssss-image-feed-list:empty { display: none } to get rid of this area:
    image

  • You can make the width/height > 100% of the window and you can then loose the resize/settings and get stuck! 😅

Otherwise its really great, and the new settings popup is really nice having it all in one place and works well

@birdddev
Copy link
Contributor Author

Yeah, I saw the problems with the top/bottom modes.
I just don't have the time at the moment to deal with it anymore. If you think you can take care of it before this wednesday (I don't know if I'll have time by even then tbh). I am totally down for you pulling this and then pushing fixes for these things quickly right after if you like, just so people can have these improvements sooner than later.
Especially if you know how to fix them rn. I don't think I'll add any more features to this with this PR, so any fixes or patches after what's here is viable.
I just haven't been able to look in to it yet 😔

In my experience I haven't been able to drag the panel larger than the window since that's just not possible lmao, but that might be because Wayland just doesn't allow that? windows/x11/mac probably allow events outside the window, so that's probably something I just can't test for. Using min(max()) in the panel size equations should fix that probably, like how I was using it before.

@andreszs
Copy link
Contributor

andreszs commented Dec 19, 2024

Now that there's a ComfyUI built-in image queue feed, this PR should be closed and the custom feed removed from the plugin.

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.

3 participants