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

swap quotes instead of replace #2

Merged

Conversation

w-e-w
Copy link
Contributor

@w-e-w w-e-w commented Jan 4, 2024

this is inconsequential but I kind of cheap out a bit when I was implementing the infotext

I use a character replace of double quotes to single quotes as opposed to a character swap
which is totally fine as I assume that the values will only be sanitized values with no single quotes is used

but in the future if you decide for whatever reason single is used quotes infotext, it will break

in the PR I did a simple remedy by using str.translate to swap single quotes and double quotes
this way for whatever reason if in the future you do indeed wants to put a single quote inside there will be no issue

there's no compatibility issues


I think I forgotten to explain why I replaced up because with single quotes in the first place

the reason is that the infotext string would be serialized as json string "again" by webui
as json string format double quotes will have to be escaped by using a backslash " -> \"
and would cause the infotax to be messy

if the quotes are not swapped you will end up with something like this

CHG: "{\"RegS\": 0.7, \"RegR\": 1.28, \"MaxI\": 23, \"NBasis\": 2, \"Reuse\": 0.05, \"Tol\": -4.2, \"IteSS\": 0.82, \"ASpeed\": 0.4, \"AStrength\": 0.76, \"AADim\": 2}"

compared to if we swap them you will get a much clearer infotext

CHG: "{'RegS': 0.7, 'RegR': 1.28, 'MaxI': 23, 'NBasis': 2, 'Reuse': 0.05, 'Tol': -4.2, 'IteSS': 0.82, 'ASpeed': 0.4, 'AStrength': 0.76, 'AADim': 2}"

also a FYI
if you wish to compact the json string even more by removing the spaces, you can do so specifying to separator arg for json.dumps

json.dumps({'a':[1,2], 'b': 'bbb'}, separators=(',', ':'))
# {"a":[1,2],"b":"bbb"}'

but you would lose a bit of readability

"{'RegS':0.7,'RegR':1.28,'MaxI':23,'NBasis':2,'Reuse':0.05,'Tol':-4.2,'IteSS':0.82,'ASpeed':0.4,'AStrength':0.76,'AADim':2}"

if you wish to take it a step further
you can even use CSV string and split key value colon

and if you don't care about readability at all, you can even use a list with out the key
note not advisable as without the keys you have to make sure that the order of the args never change

note: in my opinion how it is now is totally fine I'm just providing information because I'm "bored"

@scraed
Copy link
Owner

scraed commented Jan 5, 2024

@w-e-w, thank you for pointing out the issue with quotes! I would like to verify my understanding of the case with you.

Firstly, if no replacement or swap occurred, the JSON string would be full of double quotes, which hampers readability.

Previously, the JSON string was written with all double quotes replaced by single quotes to enhance readability. However, this would cause problems when loading it back.

Now, the "replace" action has been changed to "swap." When writing the JSON, single quotes are replaced by double quotes and vice versa. Reading the JSON also performs the same swap, so the JSON is swapped twice, resulting in the original JSON and thus avoiding any loading errors.

In this scenario, if the JSON string contains double quotes, they will be displayed as single quotes, which is cleaner. However, single quotes in the JSON will be displayed as double quotes, which is a compromise we make to prevent loading issues.

Could you check if I have understood the situation correctly before I merge the PR?

@w-e-w
Copy link
Contributor Author

w-e-w commented Jan 5, 2024

yes you understand it is correct

I want to clarify that the current implementation does not have an issue and the pr does not change the behavior, this just makes the code more robust in the future that's likely will never happen

(and to fix my OCD)

previously I was working under the assumption that you will never single quote have in this extensions infotext
which is a totally valid assumption, as has you only have keys and float and so there never have single quotes
as such using replace is totally okay

but in the future if for whatever reason you decide to add a single quote inside it will be broken (you can just fix it then if you want)
this PR just makes it safe using the proper method so that I can sleep at night

I prepared a simplified version on how webui writes and parses infotext

I added a toggle use_quote_swap = True for you to test if quote is swapped so you can easily experiment with

import json

# https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/cf2772fab0af5573da775e7437e6acdca424f26e/modules/generation_parameters_copypaste.py#L40
def quote(text):
    if ',' not in str(text) and '\n' not in str(text) and ':' not in str(text):
        return text

    return json.dumps(text, ensure_ascii=False)

# https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/cf2772fab0af5573da775e7437e6acdca424f26e/modules/generation_parameters_copypaste.py#L47
def unquote(text):
    if len(text) == 0 or text[0] != '"' or text[-1] != '"':
        return text
    try:
        return json.loads(text)
    except Exception:
        return text


quote_swap = str.maketrans('\'"', '"\'')

if __name__ == '__main__':
    use_quote_swap = True
    
    # this section is done by extension
    print(f'\n{"-"*15}extension add infotext{"-"*15}\n')
    extension_args = {'RegS': 0.7, 'RegR': 1.28, 'MaxI': 23, 'NBasis': 2, 'Reuse': 0.05, 'Tol': -4.2, 'IteSS': 0.82, 'ASpeed': 0.4, 'AStrength': 0.76, 'AADim': 2}
    # you normally cannot store dictionary in webui infotext you have to convert it to json string (or some other format) otherwise webui will not read it properly
    extension_args_json = json.dumps(extension_args)
    if use_quote_swap:
        extension_args_json = extension_args_json.translate(quote_swap)
    print(extension_args_json)
    
    # this section is done by webui
    print(f'\n{"-"*15}webui write infotext{"-"*15}\n')
    value_str = str(extension_args_json)
    webui_infotext_key_value = quote(value_str)
    # this is string write to infotext
    print(webui_infotext_key_value)
    
    # writes to image
    
    # this section is done by webui
    print(f'\n{"="*15}webui pares infotext{"="*15}\n')
    parsed_infotext = unquote(webui_infotext_key_value)
    print(parsed_infotext)
    
    # this section is done by extension
    print(f'\n{"-" * 15}extension pares infotext{"-" * 15}\n')
    # script_callbacks.on_infotext_pasted allows extension to add additional processing to infotext
    # I add pares_infotext function to json string back to a dict
    if use_quote_swap:
        parsed_infotext = parsed_infotext.translate(quote_swap)
    parsed_infotext = json.loads(parsed_infotext)
    print(parsed_infotext)

without quote swap

"{\"RegS\": 0.7, \"RegR\": 1.28, \"MaxI\": 23, \"NBasis\": 2, \"Reuse\": 0.05, \"Tol\": -4.2, \"IteSS\": 0.82, \"ASpeed\": 0.4, \"AStrength\": 0.76, \"AADim\": 2}"

with quote swap

"{'RegS': 0.7, 'RegR': 1.28, 'MaxI': 23, 'NBasis': 2, 'Reuse': 0.05, 'Tol': -4.2, 'IteSS': 0.82, 'ASpeed': 0.4, 'AStrength': 0.76, 'AADim': 2}"

exter note

note during infotext write, it might be possible to directly use dict -> str saving the step of serialize to json and quote swap
but I don't feel safe doing so, as I;am not sure if there's an edge case that would cause a different behavior so I use json to be safe


if you wish to learn about the full process of how webui writes and parse infotext here are the relevant codes

https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/cf2772fab0af5573da775e7437e6acdca424f26e/modules/processing.py#L656

https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/cf2772fab0af5573da775e7437e6acdca424f26e/modules/generation_parameters_copypaste.py#L211

you can put a debugger breakpoint at these functions

@scraed
Copy link
Owner

scraed commented Jan 5, 2024

Nice, now I think I understand the case. I will merge the PR. Thanks for your effort!

@scraed scraed merged commit 312c8f8 into scraed:main Jan 5, 2024
@w-e-w
Copy link
Contributor Author

w-e-w commented Jan 5, 2024

thanks for putting up with my OCD 😜

@w-e-w
Copy link
Contributor Author

w-e-w commented Jan 5, 2024

note during infotext write, it might be possible to directly use dict -> str saving the step of serialize to json and quote swap
but I don't feel safe doing so, as I;am not sure if there's an edge case that would cause a different behavior so I use json to be safe

just realized why you can't do so, in python dict -> str, Boolean values are converted to string as True and False however in json it is true and false

well I guess you technically can if you know that there won't be any Boolean values

@scraed
Copy link
Owner

scraed commented Jan 5, 2024

Nice!

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