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

Implement logic for deciding when to use FreeType #2236

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gasull
Copy link

@gasull gasull commented Apr 8, 2021

This implements the logic recommended by @EchterAgo.

I don't have a Windows VM to test this out. So please run the code first.

Related PR: #2235


This change is Reviewable

@gasull gasull mentioned this pull request Apr 8, 2021
@EchterAgo
Copy link

I'll do a Windows .exe build of this and test it.

@EchterAgo
Copy link

The check seems wrong. It will use (S) when using FreeType on Windows. I think this should be correct:

    @staticmethod
    def _get_schnorr_sigil() -> str:
        """Get the right symbol for the platform"""
        should_use_freetype = False
        if sys.platform in {"cygwin", "win32"}:
            config = get_config()
            if config is not None:
                windows_qt_use_freetype = config.get("windows_qt_use_freetype")
                should_use_freetype = bool(windows_qt_use_freetype)
            if not should_use_freetype:
                # On Qt for Windows the 'ⓢ' symbol looks aliased and bad. So we
                # do this for windows.
                return "(S)"
        # On Linux & macOS it looks fine so we go with the more fancy unicode
        return "ⓢ"

@EchterAgo
Copy link

Yea, the check is wrong. Using the code above it works correctly.

@gasull
Copy link
Author

gasull commented Apr 9, 2021

@EchterAgo , I fixed the code as you indicated. Please test it again.

I wonder if I should git squash all the commits.

The default execution branch is when FreeType is being used, so
it makes sense that `should_use_freetype` is initialized to `True`.
@cculianu
Copy link
Collaborator

cculianu commented Apr 9, 2021

Any chance we can just save this as an attribute in __init__? E.g. call this once: self._schnorr_sigil = self._get_schnorr_sigil()? Just avoids having to do the same work over and over again for a function that likely will not return anything different each time.

@EchterAgo
Copy link

https://ec.loping.net/4.2.4-58-g95fe9a4c9/

Windows without FreeType:

image

Windows with FreeType:

image

I agree with @cculianu that the sigil should only be determined once on startup. FreeType can not be enabled/disabled without restarting the process.

@gasull
Copy link
Author

gasull commented Apr 12, 2021

Any chance we can just save this as an attribute in __init__?

Done.

I'm not sure if it matters where in __init__ I should call _get_schnorr_sigil(). I did it at the end of it.

@EchterAgo
Copy link

When opening the TX dialog:

Traceback (most recent call last):
  File "C:\dev\Electron-Cash\electroncash_gui\qt\history_list.py", line 181, in on_doubleclick
    self.parent.show_transaction(tx, label)
  File "C:\dev\Electron-Cash\electroncash_gui\qt\main_window.py", line 1067, in show_transaction
    d = show_transaction(tx, self, tx_desc)
  File "C:\dev\Electron-Cash\electroncash_gui\qt\transaction_dialog.py", line 55, in show_transaction
    d = TxDialog(tx, parent, desc, prompt_if_unsaved)
  File "C:\dev\Electron-Cash\electroncash_gui\qt\transaction_dialog.py", line 143, in __init__
    self.add_io(vbox)
  File "C:\dev\Electron-Cash\electroncash_gui\qt\transaction_dialog.py", line 545, in add_io
    _('{} = Schnorr signed').format(self._schnorr_sigil)
AttributeError: 'TxDialog' object has no attribute '_schnorr_sigil'

@cculianu
Copy link
Collaborator

Yeah the attribute needs to be saved earlier in the __init__ function -- namely before anything like self.update() runs. Probably should be done before any other member function is called...

@gasull
Copy link
Author

gasull commented Apr 27, 2021

Yeah the attribute needs to be saved earlier in the __init__ function -- namely before anything like self.update() runs. Probably should be done before any other member function is called...

Done.

@gasull
Copy link
Author

gasull commented May 27, 2021

@cculianu @EchterAgo Is this good to be merged?

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.

None yet

3 participants