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

Include legacy detection #159

Merged

Conversation

MatthiasZepper
Copy link
Contributor

Addresses issue #93.

Suggested changes:

  • Introduce a new variable dds_on_legacy_console in the __init__.py that stores the return value of rich.console.detect_legacy_windows(). Otherwise the detection would need to occur every time the method TextHandler.task_name is called.
  • This variable is imported into text_handler.py and a different dictionary is used to resolve the step, which contains spelled out status messages instead of emojis.

Testing:

  • Tested to the extent that the dds command doesn't crash right away.
  • rich.console.detect_legacy_windows() returns TRUE on a Windows 8 cmd.exe (tested before with a separate example script).
  • So far, I have not tested if and how the alternative messages are displayed on a legacy console as that would require the whole Docker dds_web backend to be run inside the Virtual Machine. Will wait to test this until we have a public API up and running on the web.

Subject to further discussion:

  • Naming of the alternative status messages: encrypt and decyrpt were replaced by seal and open respectively to shorten both to 4 letters. Alternatively, something like ecpt/dcpt or ecph/dchp (encipher/decipher) could be used.
  • Formatting of the status messages: Bold or anything else like a specific color?

For the record:

  • rich.console.detect_legacy_windows() is unfortunately only a proxy to detect if emojis will be displayed correctly. Unicode support is mandatory and font fallback required, if the chosen font doesn't have glyphs for the respective emojis. Windows legacy consoles never support font fallback and rarely Unicode. Font fallback might be an issue on other terminal emulators and operating systems, too and is not tested for with above condition.
  • Emojis are also used at other parts of the code, e.g. in the exceptions.py. Currently they are not stripped from the output there, so the current pull request does not fully resolve Fix Windows icon compatibility  #93 .

@alneberg
Copy link
Contributor

I think the changes in .gitignore would be fine to keep for everyone so don't bother changing those. 👍

dds_cli/text_handler.py Outdated Show resolved Hide resolved
@i-oden
Copy link
Member

i-oden commented Sep 27, 2021

@MatthiasZepper Other than the requested change it looks good!

@i-oden
Copy link
Member

i-oden commented Sep 27, 2021

@MatthiasZepper Run black . in your dds_cli folder, or if you have VSCode you can also set black to be automatically run on save

@MatthiasZepper
Copy link
Contributor Author

@MatthiasZepper Run black . in your dds_cli folder, or if you have VSCode you can also set black to be automatically run on save

@inaod568 Beyond comprehension: Configured everything together with Johannes last week, so that black runs automatically with every time a file is saved in VS Code. This also worked on last Thursday and Friday. Apparently MacOS update also broke that and I didn't notice, because the other stuff in VSCode like the extensions still worked like a charm.

dds_cli/text_handler.py Outdated Show resolved Hide resolved
@i-oden
Copy link
Member

i-oden commented Sep 27, 2021

@MatthiasZepper Run black . in your dds_cli folder, or if you have VSCode you can also set black to be automatically run on save

@inaod568 Beyond comprehension: Configured everything together with Johannes last week, so that black runs automatically with every time a file is saved in VS Code. This also worked on last Thursday and Friday. Apparently MacOS update also broke that and I didn't notice, because the other stuff in VSCode like the extensions still worked like a charm.

Ah yeah, could also be a VSCode thing. It does weird things some times. But great that it's working again / that black has been run again 👍🏻

Copy link
Member

@i-oden i-oden left a comment

Choose a reason for hiding this comment

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

rich handles the display witdh etc automatically, so the extra 3 characters here really shouldn't affect this. The only thing that will happen is that the actual progress bar will become shorter. So the "todo test for display width" thing is not something we need to put a lot of effort into, this should be a sufficient solution 👍🏻

dds_cli/text_handler.py Outdated Show resolved Hide resolved
@i-oden i-oden merged commit 3c5886c into ScilifelabDataCentre:dev Sep 27, 2021
@MatthiasZepper MatthiasZepper deleted the include_legacy_detection branch September 27, 2021 15:26
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.

Fix Windows icon compatibility
3 participants