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

Display improvements to PNG Info tab #16415

Open
wants to merge 20 commits into
base: dev
Choose a base branch
from

Conversation

MarcusNyne
Copy link
Contributor

Description

PNG Info tab text display currently looks like a big block of text, which makes it hard to find specific settings, and is a pain when you are trying to copy a setting. This improvement parses PNG Info settings and displays them in a format that makes it easier to read, and also allows a setting to be copied to the clipboard with a simple click.

Features

  • If there is a failure parsing, displays falls back to the old style (i.e. comfyui json)
  • High performance regex is used for parsing .. comparable performance to preexisting behavior
  • Values wrapped in quotes appear as orange, other values appear in blue
  • Colors tweaked for dark mode, so looks good in both modes
  • Click to copy .. displays CSS animation so the user knows something is happening
  • When (orange) text is copied, escaped newlines converted to newlines (i.e. adetailer prompt)
  • When an extension adds settings that are not in the expected format, non-standard settings displayed in plain text (i.e. dynamic prompt templates)

Code changes

  • Added a new module png_parser.py that contains the parsing logic, and regex expressions
    ** Non-standard settings identified by a newline outside of quotes, using dynamic prompts as a reference
  • Modified modules\extras.py run_pnginfo() to use parsing module, and convert plain text to HTML
    ** Utilized existing formatting methods when possible
    ** Introduced for formatting, copy to clipboard, and copy animations
    ** Preserved existing formatting when there is a parsing error
    ** No changes to plain text format returned by method
  • Formatting styles were added to end of style.css
    ** Uses existing CSS variables whenever possible
    ** Support for dark mode
    ** Uses @Keyframes animation
  • Added uiCopyElementText() to script.js
    ** Copies element text to clipboard
    ** Starts copy animation

Passed python and javascript linting. Passed test process.

Checklist:

Text on PNG Info tab is parsed and colorized in a way that makes it easier to read.  Parameter values can be copied by clicking on them.
- High performance regex used for parsing
- Normal values are displayed in blue, but string content is displayed in orange to improve readability (i.e. adetailer prompts)
- Clicking to copy uses a pointer cursor and a green color animation to show something is happening
- Color choices configured differently for dark mode in order to optimize readability
- Copying strings with \n automatically converts to newlines during copy operation
- Settings that don't follow normal conventions are not parsed, but displayed in the old style (i.e. dynamic prompt templates)
- If there are parsing issues (or exceptions), defaults to the old display mode
Lint run against python and javascript
@w-e-w
Copy link
Collaborator

w-e-w commented Aug 23, 2024

issue 1: unable to copy the raw unaltered infotext

with this PR if someone were to try and copy the entire infotext from HTML, it would introduce extra new lines that weren't therethere
sometimes the new lines are harmless but it can also cause certain syntax to break

you could try to make it so the HTML stylization does not alter the copy text
or it would be simpler to just add a button or something to copy the raw infotext

Issue 2: parsing breaks when no Negative prompt

with Negative prompt
image
no Negative prompt:
image

yes both screenshot is of this PR

@MarcusNyne
Copy link
Contributor Author

@w-e-w , thank you for taking the time to review my PR. I fixed the problem with no negative prompt. I like the idea of adding a way to copy text, but want to make sure I understand the problem you are describing. I copied the entire text manually and I think the newlines make the info text more readable, and so are beneficial. What do you mean by "it can also cause certain syntax to break"?

Thanks!

- Handled no negative prompt scenario
- Include new lines in positive/negative prompts
- Added links to the top to copy png info sections (all/positive/negative/settings)
@w-e-w
Copy link
Collaborator

w-e-w commented Aug 24, 2024

I copied the entire text manually and I think the newlines make the info text more readable, and so are beneficial. What do you mean by "it can also cause certain syntax to break"?

readable does not mean that it's "the same"

if someone has an extension that adds extra scripting functionality to prompts input, then for there syntex newline could have special meanings, so adding a extra newline could potentially break ther syntax or change the output

there are other use cases that are very sensitive to minor changes

@MarcusNyne
Copy link
Contributor Author

@w-e-w , I followed your suggestion and added some links to copy text. The "All" button copies text that should be identical to the previous manual copy. Please let me know if this works?

Thanks again!

@w-e-w
Copy link
Collaborator

w-e-w commented Aug 24, 2024

found a rather hilarious issue

2024-08-25.07_11_47_017.chrome.mp4

once the copy button is clicked, it flashes every time you switch to the tab

@MarcusNyne
Copy link
Contributor Author

@w-e-w thank you, that's a good catch. I'll see if I can fix it.

@MarcusNyne
Copy link
Contributor Author

@w-e-w fixed it! Thanks again. Really appreciate that you are finding these issues.

All to Copy
Positive to Prompt
@MarcusNyne
Copy link
Contributor Author

@w-e-w , renamed some of the quick links to make it more obvious what is going on. Finished with all changes, now.

@w-e-w w-e-w force-pushed the m9-240816-pnginfo-text-copy branch from 3dc095c to 9fb7b07 Compare October 20, 2024 01:28
Copy link
Collaborator

@w-e-w w-e-w left a comment

Choose a reason for hiding this comment

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

found a bug
it seems to be removeing / converting double space to single space

@w-e-w w-e-w marked this pull request as draft October 20, 2024 01:49
@w-e-w w-e-w marked this pull request as ready for review October 20, 2024 02:26
@w-e-w
Copy link
Collaborator

w-e-w commented Oct 20, 2024

seems to be working correctly now
the white spaces were removed by the browser, fix by adding css rule

while personally like the changes in general but as this maybe controversial so like more feedback before merging

one thing I don't like is the italic fonts for geninfo-setting-string (parameters that starts with ")

.pnginfo-page p span.geninfo-setting-string {
color: var(--pnginfo-string-color);
font-style: italic;
cursor: pointer;
}

italic (current)
image
normal
image

@MarcusNyne
Copy link
Contributor Author

@w-e-w , thank you for following up. Yes, I understand that the appeal of visual and GUI changes can be subjective and people can have strong opinions about it. I tried to make the text formatting as nice as possible for light and dark modes. I wouldn't expect them to appeal with everyone, but I think even if someone doesn't like all my aesthetic choices, hopefully they feel that it is better than the wall of text we have now. I have been using the changes for about 2 months now with no issues for my prompts. Thanks again.

def init_re(cls):
if cls.re_top_level is None:
cls.re_top_level = re.compile(r'^(?P<positive>(?:.|\n)*)\nNegative prompt: (?P<negative>(?:.|\n)*)\n(?=Steps: )(?P<parameters>(?:.|\n)*)$')
cls.re_top_level2 = re.compile(r'^(?P<positive>(?:.|\n)*)\nSteps: (?P<parameters>(?:.|\n)*)$')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I broke the parser
let me see if I can fix it

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. when no Negative prompt, Stpes is not in the parameters group and so is not displayed and it's not copied when using copy buttons
  2. the regex pattern is using a dangerous assumption that Steps must exist and be is the first listed parameter

def pnginfo_format_quicklink(name):
return f"<span class='geninfo-quick-link' onclick='uiCopyPngInfo(this, \"{name}\")'>[{html.escape(name)}]</span>"


def run_pnginfo(image):
if image is None:
return '', '', ''

geninfo, items = images.read_info_from_image(image)
Copy link
Collaborator

@w-e-w w-e-w Oct 21, 2024

Choose a reason for hiding this comment

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

another issue: items is discarded if parser.valid

these are contains extra information that might be added on by stuff such as post-processing on extras tab
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@w-e-w Thank you for taking the time to go over this. I will take the time to look over the changes and test. It will take a few days. Thank you again!

w-e-w added 6 commits October 21, 2024 13:41
extract spitting of infotext converting parsing parameters of as fractions so they can be reused elsewhere
split_infotext(str)
parameters_to_dict(str)
parse_parameters(dict)

fix edge case issue with negative prompt parsing
allowing user choose between new and old styles of HTML for PNG info page
@w-e-w
Copy link
Collaborator

w-e-w commented Oct 21, 2024

@MarcusNyne I fix the issues that I mentioned and added an an option for one to switch back to the old stye, can you have a test

Settings > Infotext > PNG Info Sytle
image

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