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

Deprecate theme.FyneLogo() function #3705

Merged
merged 6 commits into from
Apr 1, 2023

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Mar 5, 2023

Description:

This deprecates the theme.FyneLogo() function as discussed on a call a few months back. Developers should use their own application icons instead. Anyone that needs a Fyne logo can bundle it manually.

Fixes #3296

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Any breaking changes have a deprecation path or have been discussed.

@coveralls
Copy link

coveralls commented Mar 5, 2023

Coverage Status

Coverage: 61.776% (+0.004%) from 61.772% when pulling 1b52d05 on Jacalz:deprecate-fynelogo into 0e70551 on fyne-io:develop.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

A couple of minor things inline. But yes we need to figure what the default for tray/app icon is and keep it consistent. I'm not sure that no-icon works in the tray context?

@@ -1,47 +1,33 @@
<canvas size="200x300">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should have been committed - it is part of a flakey test that needs more work

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I made sure to restore it

@andydotxyz
Copy link
Member

I think it needs the FyneApp.toml references updated too

@Jacalz
Copy link
Member Author

Jacalz commented Mar 12, 2023

But yes we need to figure what the default for tray/app icon is and keep it consistent. I'm not sure that no-icon works in the tray context?

Absolutely. You are right. It is currently changed to use the warning icon but the error icon could work as well. Another idea of mine was to use the question icon. What would you prefer?

vcscsvcscs
vcscsvcscs previously approved these changes Mar 16, 2023
Copy link

@vcscsvcscs vcscsvcscs left a comment

Choose a reason for hiding this comment

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

LGTM

@andydotxyz
Copy link
Member

But yes we need to figure what the default for tray/app icon is and keep it consistent. I'm not sure that no-icon works in the tray context?

Absolutely. You are right. It is currently changed to use the warning icon but the error icon could work as well. Another idea of mine was to use the question icon. What would you prefer?

Maybe instead of re-using we should actually have an "icon missing" icon? then app developers can replace it using the theme without changing one of those dialog icons...

@Jacalz
Copy link
Member Author

Jacalz commented Mar 17, 2023

That sounds like a good plan. I'll go ahead with getting that sorted in a few days

@Jacalz
Copy link
Member Author

Jacalz commented Mar 29, 2023

I can't seem to find any icon in material design to indicate that an icon is missing. It does put a stick in the wheel so to say.
There are a few options that we should consider:

  1. Does someone else have more luck with finding one? :)
  2. Should we consider any other icons from what we already have? Suitable ones would be Cancel, Warning or Question.
  3. If both above are not possible, should we perhaps abandon this?

@andydotxyz
Copy link
Member

There is also HelpIcon which is (currently) more filled in and might make a better fallback icon?

@andydotxyz
Copy link
Member

@Jacalz Jacalz force-pushed the deprecate-fynelogo branch from 231d8a5 to e55cff3 Compare March 31, 2023 17:30
@Jacalz
Copy link
Member Author

Jacalz commented Mar 31, 2023

This is what we have used in Fyne Desk when an app has no icon https://fonts.google.com/icons?selected=Material%20Symbols%20Outlined%3Abroken_image%3AFILL%400%3Bwght%40400%3BGRAD%400%3Bopsz%4048

Thanks. That's wonderful. I was trying to find icons with missing but not broken when I was looking. That icon will do nicely.

@Jacalz
Copy link
Member Author

Jacalz commented Mar 31, 2023

I had to force-push a rebase on develop. Something seemed strange with the test where it expected foreground_ at the beginning of the name yet none of the other files had it.

@Jacalz Jacalz requested a review from andydotxyz March 31, 2023 17:41
@andydotxyz
Copy link
Member

I had to force-push a rebase on develop. Something seemed strange with the test where it expected foreground_ at the beginning of the name yet none of the other files had it.

The foreground_ prefix should be on every default themed resource name now, it was a fix to a cache overlap bug earlier this month

@Jacalz
Copy link
Member Author

Jacalz commented Apr 1, 2023

Yeah, I remember it being added but the test were strange. None of the other icon tests included the change so I didn’t either. However, my code still failed because it was missing foregroundz. Strange. Either way, this should be fine now :)

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Nice

@Jacalz Jacalz merged commit c5048b4 into fyne-io:develop Apr 1, 2023
@Jacalz Jacalz deleted the deprecate-fynelogo branch April 1, 2023 10:28
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.

4 participants