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

SeedSigner Icons v2 #423

Merged
merged 30 commits into from
Aug 8, 2023
Merged

SeedSigner Icons v2 #423

merged 30 commits into from
Aug 8, 2023

Conversation

easyuxd
Copy link
Contributor

@easyuxd easyuxd commented Aug 4, 2023

This PR is for a v2.0 update to the SeedSigner Icon Set. @jdlcdl recently brought to my attention that the icon set is in need of an update, as it has not been updated since the v0.5.0 release.

I could use help with additional testing to ensure I didn’t break anything with this change. Especially with icon sizing (it looks like the SeedSigner icons draw smaller than FontAwesome icons). I've outlined a couple of visual defects below.

Huge thanks to @jdlcdl for the help troubleshooting this PR -- it would have been impossible without him. I’ve included his modified version of the @kdmukai screenshot_generator tool. It's hugely useful for testing, with really solid improvements. This can be removed if we don’t want to revive it right now.


Screenshots:

Home Screen Before Home Screen After

Full list of icons

[SeedSigner Design System (v0.6)]
(https://github.com/easyuxd/seedsigner-design-system#iconography)

Changes:

  • Renamed seedsigner-glyphs.otf to seedsigner-icons.otf
  • Updated seedsigner-icons.otf
  • Renamed SeedSignerCustomIconConstants to SeedSignerIconConstants
  • Updated SeedSignerIconConstants names and Unicode values in src/seedsigner/gui/components.py
  • Updated "blue" icon color to "0084FF" for better contrast

Defects:

  • Resolved: New icons draw smaller on the Home Screen grid tiles. I could use help increasing their size to 40x40px.

  • QR Brightness overlay - New up/down chevrons may have affected padding @overcat

- Renamed `seedsigner-glyphs.otf` to `seedsigner-icons.otf`
- Updated `seedsigner-icons.otf`
- Renamed `SeedSignerCustomIconConstants` to `SeedSignerIconConstants`
- Updated `SeedSignerCustomIconConstants` names and Unicode values in src/seedsigner/gui/components.py
@@ -28,7 +28,7 @@ class GUIConstants:
REGTEST_COLOR = "#00caf1"

ICON_FONT_NAME__FONT_AWESOME = "Font_Awesome_6_Free-Solid-900"
ICON_FONT_NAME__SEEDSIGNER = "seedsigner-glyphs"
ICON_FONT_NAME__SEEDSIGNER = "seedsigner-icons"
ICON_FONT_SIZE = 22
ICON_INLINE_FONT_SIZE = 24
ICON_LARGE_BUTTON_SIZE = 36
Copy link

Choose a reason for hiding this comment

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

For your LargeButtonScreen where your latest icon set appears smaller than before, you might play with resizing this from 36 to something bigger.

I don't know what bad side-effects this might have. @kdmukai will probably know better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 48 and it's fixed. I can't find anything that broke.
MainMenuView

@jdlcdl
Copy link

jdlcdl commented Aug 5, 2023

This morning I had a chance, with fresh eyes and coffee, to really take a look at these changes. I hear "Try a different color shirt with those!" almost daily, so forgive me for not having noticed the improvements at first glance yesterday.

On a large desktop screen with 2 browsers open side by side:

  • a link to my branch as the BEFORE
  • and the link to yours, as the AFTER

...it was much easier for me to take my time and appreciate your fine work.

@kdmukai really did wonderful work with the screenshots_generator, they're so useful: like acceptance testing for folks with a sense of style (who can help folks like me picking clothes).

Over time, we'll do better to keep these updated and get more screens working.

@easyuxd
Copy link
Contributor Author

easyuxd commented Aug 5, 2023

@jdlcdl

Haha. Thank you for the kind words, and for the before and after. 🙏

I added links and thumbnails to the before and after at the top.

@kdmukai
Copy link
Contributor

kdmukai commented Aug 5, 2023

Omigosh this makes me so happy to see the screenshot generator revived and being so useful!!

Looks like the ToolsAddressExplorerAddressTypeView demo data could probably use some tweaking (that screen should also be displaying the fingerprints of each signer). Will also have to check if the centering if just off because of the lack of fingerprints.

@newtonick
Copy link
Collaborator

newtonick commented Aug 7, 2023

When I run the screenshot generator pytest it does not appear to work. I get this warning.
image

Any ideas on how to resolve?

The normal unit test suite passes without issue.
image

@jdlcdl
Copy link

jdlcdl commented Aug 7, 2023

When I run the screenshot generator pytest it does not appear to work. I get this warning.

I'm aware of this warning and I have not yet tracked it down. Otherwise, on the pi from where you ran it, there should be a ../seedsigner-screenshots directory with a README.md and many pngs, if it did anything.

While I was working on this branch here for @easyuxd to have a tight feedback loop while implementing his new icon set, I made many less-than-ideal hacks just to get these working... to that end.

I believe that the screenshots code (everything in tests/screenshot_generator/) is NOT yet ready for merge. I was going to suggest to easyuxd that as a last commit to this pr, he perform:

git rm -r tests/screenshot_generator

(or for you to do similar during merge) so that this code is removed from this particular pr, but so that it remains available for him to use in order to see his design changes at will.

That would afford me the time to do some much needed cleanup, as well, to get feedback from @kdmukai who is sure to have plenty of good ideas, having created it. I suspect it will play well with the recent flow-tests changes.

If the tests/screenshots_generator does get merged, it will become part of the codebase and I will then issue PRs with any cleanup that I have, and Keith and others will be able to do so too. Regardless, since these reside outside of src/ they do not get in the way of a 0.7.0 release because they won't make it into the image. I believe that everything else easyuxd has done in the pr might still be worthy of making it into 0.7.0 and I'd rather focus on those as a priority (perhaps: like a GUIConstants.INFORMATIONAL attribute to express #0084ff, so that we'll find it instead of using "blue". We could issue a PR for that later as well).

@easyuxd, if you'd like help removing the screenshots_generator code from this pr so that its lack of readiness won't delay the rest of this pr, please let me know.

@jdlcdl
Copy link

jdlcdl commented Aug 7, 2023

Looks like the ToolsAddressExplorerAddressTypeView demo data could probably use some tweaking (that screen should also be displaying the fingerprints of each signer). Will also have to check if the centering if just off because of the lack of fingerprints.

Wow, nice catch! The left-aligned text didn't even catch my eye, nor that something is missing. These are the two screens that appear directly after scanning a wallet descriptor from "tools / address explorer / scan wallet descriptor".

Imagine much prettier than these photos... but they still reflect the layout and info on screen.

ss_taestv1
button in above image is "Continue".

ss_taestv2
buttons in above image are "Receive addresses" and "Change addresses"

@newtonick newtonick added the enhancement New feature or request label Aug 7, 2023
@newtonick
Copy link
Collaborator

I'm aware of this warning and I have not yet tracked it down. Otherwise, on the pi from where you ran it, there should be a ../seedsigner-screenshots directory with a README.md and many pngs, if it did anything.

Thank you! I was looking in the seedsigner repo folder and did not expect it to be written one level higher.

@kdmukai
Copy link
Contributor

kdmukai commented Aug 7, 2023

The one weirdo warning:

tests/screenshot_generator/generator.py::test_generate_screenshots
  /Users/kdmukai/.virtualenvs/seedsigner-env/lib/python3.10/site-packages/_pytest/threadexception.py:75: PytestUnhandledThreadExceptionWarning: Exception in thread Thread-26
  
  Traceback (most recent call last):
    File "/opt/homebrew/Cellar/python@3.10/3.10.12/Frameworks/Python.framework/Versions/3.10/lib/python3.10/threading.py", line 1016, in _bootstrap_inner
      self.run()
    File "/Users/kdmukai/dev/seedsigner/src/seedsigner/gui/screens/screen.py", line 185, in run
      renderer.show_image()
    File "/Users/kdmukai/dev/seedsigner/tests/screenshot_generator/utils.py", line 54, in show_image
      raise ScreenshotComplete()
  tests.screenshot_generator.utils.ScreenshotComplete
  
    warnings.warn(pytest.PytestUnhandledThreadExceptionWarning(msg))

-- Docs: https://docs.pytest.org/en/stable/warnings.html

was driving me crazy. Turns out that it's from ToolsAddressExplorerAddressListView because that spins up a separate thread for LoadingScreenThread. I haven't thought about how to catch that exception.

It doesn't do any harm, but is annoying to see the warning.

@jdlcdl
Copy link

jdlcdl commented Aug 7, 2023

As of cd65820,

ACK tested. I take it back, merge conflicts and tests that are not passing, but we're close.

All images are being generated (though more could be generated and plenty of organization to do). These look really nice. You probably don't know this, but sometimes I find myself just scrolling up and down these screenshots, thinking "Wow... it really is a nice interface!"

p.s. If you notice an initial delay, as if pr 416 got backed-out, it's just that it isnt in this branch yet.

@easyuxd
Copy link
Contributor Author

easyuxd commented Aug 7, 2023

Thank you, @jdlcdl and @kdmukai

@jdlcdl Are the conflicts just from changing SeedSignerCustomIconConstants to SeedSignerIconConstants? I should be able to mark as resolved, no?

@jdlcdl
Copy link

jdlcdl commented Aug 8, 2023

@jdlcdl Are the conflicts just from changing SeedSignerCustomIconConstants to SeedSignerIconConstants? I should be able to mark as resolved, no?

Yes, there are 4 conflicts currently and they're mostly related to the change of the constant name, you'd want to select your changes and remove the others. However, in the psbt_views.py conflict, there is also a recent bug fix where you want a little of both fixes: your icon constants of course, but the way 'dev' refers to "buttons" instead of "self.buttons" needs to be applied to yours, else a recent bug recently fixed soon returns.

@jdlcdl
Copy link

jdlcdl commented Aug 8, 2023

As of f6c1152,

All tests are passing whether run on raspi or desktop.
All screenshots, that are known to run, are working on raspi as well as desktop, which is really nice!

@jdlcdl
Copy link

jdlcdl commented Aug 8, 2023

8bb6d25 almost brought a tear to my eye. <3

@easyuxd
Copy link
Contributor Author

easyuxd commented Aug 8, 2023

@newtonick
Conflicts resolved, with the help of @jdlcdl.

@jdlcdl
Copy link

jdlcdl commented Aug 8, 2023

ACK tested

viewed before and afters as of d27e9e3. Nice job on the merge conflicts, and especially your latest cleanup-commits, craftsman!

@newtonick
Copy link
Collaborator

ACK Tested

@newtonick newtonick merged commit b01a5b6 into SeedSigner:dev Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

4 participants