Feature/metadataapi #2813

andydotxyz · 2022-02-28T19:21:09Z

Add support for reading metadata through API.
More work to follow, including tests. Marked draft for discussion.

Fixes #1940 (indirectly)

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.
Public APIs match existing style.

Fixes fyne-io#1940

lucor

Just a small typo, apart that LGTM

cmd/fyne/internal/commands/build.go

Jacalz

This looks very promising to me. I know that this is a draft but I just had two questions inline.

cmd/fyne/internal/commands/build.go

app/meta.go

cmd/fyne/internal/commands/build.go

Fix typo Co-authored-by: Luca Corbo <lu.corbo@gmail.com>

coveralls · 2022-03-01T10:52:52Z

Coverage increased (+0.04%) to 61.874% when pulling a1112e3 on andydotxyz:feature/metadataapi into 8dd874b on fyne-io:develop.

app.go

….toml

andydotxyz · 2022-03-01T21:39:19Z

I cannot figure out these test failures. They are passing correctly on my fork but failing here (and pass on my desktop too).
The report is specifying line numbers that don't exist (400 something on package_test.go that has just 180).
Also in my checkout / fork the test "Test_PackageWasm" does not appear, yet that is failing.
Anyone got an idea what is happening?

Bluebugs · 2022-03-01T22:32:00Z

I cannot figure out these test failures. They are passing correctly on my fork but failing here (and pass on my desktop too). The report is specifying line numbers that don't exist (400 something on package_test.go that has just 180). Also in my checkout / fork the test "Test_PackageWasm" does not appear, yet that is failing. Anyone got an idea what is happening?

This is very weird, but I have the impression that it is actually merging with develop and running the tests from develop. If you do a manual merge locally, you should be able to see them too.

andydotxyz · 2022-03-01T22:43:09Z

I cannot figure out these test failures. They are passing correctly on my fork but failing here (and pass on my desktop too). The report is specifying line numbers that don't exist (400 something on package_test.go that has just 180). Also in my checkout / fork the test "Test_PackageWasm" does not appear, yet that is failing. Anyone got an idea what is happening?

This is very weird, but I have the impression that it is actually merging with develop and running the tests from develop. If you do a manual merge locally, you should be able to see them too.

Hmm, I thought I had done so, but maybe a day or two back was not enough ;)

andydotxyz · 2022-03-01T22:58:43Z

This is very weird, but I have the impression that it is actually merging with develop and running the tests from develop. If you do a manual merge locally, you should be able to see them too.

Amazing, this was bang on thanks. Fixed :)

Jacalz · 2022-03-02T04:53:57Z

I'll try to have a look at this later today or tomorrow.

Bluebugs · 2022-03-02T18:12:39Z

This might be a silly idea, but could address @Jacalz concern. What do you think of having a "fyne metadata" command line that read the metadata and generate a go file instead. This could be used then by go generate. This would make go build actually work and provide metadata too.

Jacalz · 2022-03-03T08:12:30Z

Can you elaborate a bit more in how you intend it to be used and how it is different from fyne bundle?

andydotxyz · 2022-03-03T09:51:21Z

This might be a silly idea, but could address @Jacalz concern. What do you think of having a "fyne metadata" command line that read the metadata and generate a go file instead. This could be used then by go generate. This would make go build actually work and provide metadata too.

Yes we could do this. However metadata can change (version numbers etc) and forgetting to run this would mean the version info is out of date. Also exposing the App.Metadata() API will not be possible / reliable if the packaging of this data is optional...

Bluebugs · 2022-03-03T16:45:00Z

Can you elaborate a bit more in how you intend it to be used and how it is different from fyne bundle?

By reading its information from the FyneApp.toml and generating the proper metadata structure. Very different job compared to fyne bundle, I think.

Bluebugs · 2022-03-03T16:47:01Z

However metadata can change (version numbers etc) and forgetting to run this would mean the version info is out of date. Also exposing the App.Metadata() API will not be possible / reliable if the packaging of this data is optional...

I agree there is a synchronization problem. fyne package could maybe run go generate to make sure it is in sync, but for someone using go build, they would have to do it manually and thus the risk of not being in sync. Neither are ideal solution and both have drawback. I am personally fine with both as long as there drawback are documented.

andydotxyz · 2022-03-03T18:04:34Z

However metadata can change (version numbers etc) and forgetting to run this would mean the version info is out of date. Also exposing the App.Metadata() API will not be possible / reliable if the packaging of this data is optional...

I agree there is a synchronization problem. fyne package could maybe run go generate to make sure it is in sync, but for someone using go build, they would have to do it manually and thus the risk of not being in sync. Neither are ideal solution and both have drawback. I am personally fine with both as long as there drawback are documented.

Ah cunning, combine the best of both. Inject latest into generated source code so go build still works. That sounds genius and may be possible. It might require another API because the app would have to inject the data instead of using the compiler. Will have to think about this more to see if it can be done more cleanly.

andydotxyz · 2022-03-21T11:54:14Z

This might be a silly idea, but could address @Jacalz concern. What do you think of having a "fyne metadata" command line that read the metadata and generate a go file instead. This could be used then by go generate. This would make go build actually work and provide metadata too.

I looked into this further and unfortunately I don't think it's practical. If the developer forgets to commit the file, and they use the go build then the compile will fail. With the ldflags approach in this PR it will not fail as the values fall back to default / "in development" info.

Jacalz · 2022-03-21T21:08:30Z

I’m genuinely sorry but I would still vote for keeping the icon part out for now and implementing it at a later date. That way we can get more time to think about a better solution that doesn’t have the current drawbacks.

This API will be great even without the icon. I think you've done a great job with it. It kind of falls in line with the extra build metadata that was added for Go 1.18 (that might even be useful for this in the future).

andydotxyz · 2022-03-22T11:28:09Z

Info for the 3 different builds, "basic" is just go build, "bundled" adds the icon using fyne bundle and then "meta" uses this PR. I measure the time from start to main executing and from main executing to window entering foreground. This is with a 512x512 icon, 84kB.

type	size	start time	show time
basic	-	61ms	238ms
bundle	+82kB	56ms	237ms
meta	+115kB	59ms	238ms

Although the source code is much smaller (no bundle file, but even if there was the MD5 is half the size of the bundled byte array source) the compiled image data is around 2% smaller bundled (compared with image size) vs around 30% larger than the source image due to the MD5 storage format.
However you can see that startup time is pretty much the same.

I could look at different string encodings for the image data I guess.

andydotxyz · 2022-03-22T11:41:49Z

Just a quick reminder of the current scanario:

v2.1.4 and "go build": User does no icon bundling and no custom icon is set on a window or in the app bundle
v2.1.4 with bundled icon and "go build": The users code will set the window icon but most OS will not show this for app icon.
v2.1.4 with fyne package: OS will show app icon but based on OS the window icon may still be blank
v2.1.4 with fyne package and bundled icon: Apps and windows will all have icon set.

As you can see above it basically requires user adding code into the app and bundling the icon manually to get it everywhere.

This PR and "go build": icon is empty as before
This PR and bundled icon with "go build": as above - as before, partial
This PR and "fyne package": Icon will appear in all places without extra code
This PR and "fyne package" with bundled icon: as before, all OK
New possibility with this PR and "fyne build" the icon is included by default through FyneApp.toml or -icon param

The potential downside is that users bundling their own icon manually may have it stored twice when packaging for the benefit of controlling it's manual inclusion when not packaging and using "go build" only.

I can see that this is a tradeoff, but I think the lowered inconvenience for the primary target (packaging) is worth it for the possible downside during development (avoiding fyne tools and finding the icon not inserted unless code added).

Jacalz · 2022-03-22T14:54:00Z

I think you are right. There is no better way around this. I don't know if it's worth it, but I guess we could consider using https://github.com/cristalhq/base64 if it makes any difference?

Another though that I had was if it makes sense to have an option to turn off the metadata? Something like a nometadata build tag that ignores loading it in and parsing it.

andydotxyz · 2022-03-30T14:59:25Z

I tried some other encodings but they just didn't work (either invalid for a ldflags command line parameter, or much larger than comparison).
In the process I think I found that my previous measurements were not correct. So this shows updated size comparison:

Comparing this time to metadata included - but no icon metadata:

base64 + 98k
hex - 163k

So base64 is actually less than half the increase than I though it was compared to the efficient bundling of bytes directly (which was +82k). It makes me feel a little better.

Jacalz

Looks great. This is a really nice improvement. It looks like this branch isn't updated with the fyne buildcommand but I guess that this will work it as well.

andydotxyz · 2022-04-01T12:54:45Z

It didn't but I pushed an additional minor change over the top which added metadata to the serve command as well

andydotxyz added 3 commits February 28, 2022 18:57

Initial commit for accessing app metadata through API

cdb74f2

Use metadata API to fix Windows bug

dd95366

Fixes fyne-io#1940

oops, missed the test data in commit

89f368a

andydotxyz changed the base branch from master to develop February 28, 2022 19:21

lucor suggested changes Feb 28, 2022

View reviewed changes

cmd/fyne/internal/commands/build.go Outdated Show resolved Hide resolved

Jacalz reviewed Feb 28, 2022

View reviewed changes

cmd/fyne/internal/commands/build.go Outdated Show resolved Hide resolved

app/meta.go Show resolved Hide resolved

Bluebugs reviewed Feb 28, 2022

View reviewed changes

cmd/fyne/internal/commands/build.go Outdated Show resolved Hide resolved

andydotxyz and others added 5 commits February 28, 2022 14:47

Update cmd/fyne/internal/commands/build.go

f9cb797

Fix typo Co-authored-by: Luca Corbo <lu.corbo@gmail.com>

Address comments and add test

56cd58b

Merge develop and add build tag injection to web

131e046

Fix broken align

3ddda4e

Fix output of build when no meta tags present

2092ed0

Package up icon metadata too :)

aba5eb9

Jacalz reviewed Mar 1, 2022

View reviewed changes

app.go Show resolved Hide resolved

andydotxyz added 2 commits March 1, 2022 21:09

We don't have to use the WithID constructor any more if using FyneApp…

b39daff

….toml

Fix broken align

5c1a800

andydotxyz marked this pull request as ready for review March 1, 2022 21:41

Make the ldflags parameters easier to extend

8a4d6dc

andydotxyz added 3 commits March 1, 2022 22:44

Merge branch 'develop' into feature/metadataapi

0deef8e

Fix typo

efa454b

Update to latest develop and add JS support in pacakge meta

3d5912b

Bluebugs previously approved these changes Mar 1, 2022

View reviewed changes

andydotxyz mentioned this pull request Mar 6, 2022

Add a fyne build command. #2821

Merged

4 tasks

Use injected icon for app icon by default

a1112e3

andydotxyz dismissed Bluebugs’s stale review via a1112e3 March 6, 2022 15:38

lucor approved these changes Mar 30, 2022

View reviewed changes

Jacalz self-requested a review March 30, 2022 17:33

Jacalz approved these changes Mar 31, 2022

View reviewed changes

andydotxyz merged commit facd33e into fyne-io:develop Apr 1, 2022

andydotxyz deleted the feature/metadataapi branch April 1, 2022 12:08

Jacalz mentioned this pull request Apr 21, 2022

Customize the icon and name of the notification message #2935

Open

bon-ami mentioned this pull request Jun 7, 2022

Windows missing version metadata when packaged #3046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/metadataapi #2813

Feature/metadataapi #2813

andydotxyz commented Feb 28, 2022 •

edited

Loading

lucor left a comment

Jacalz left a comment

coveralls commented Mar 1, 2022 •

edited

Loading

andydotxyz commented Mar 1, 2022 •

edited

Loading

Bluebugs commented Mar 1, 2022

andydotxyz commented Mar 1, 2022

andydotxyz commented Mar 1, 2022

Jacalz commented Mar 2, 2022

Bluebugs commented Mar 2, 2022

Jacalz commented Mar 3, 2022

andydotxyz commented Mar 3, 2022

Bluebugs commented Mar 3, 2022

Bluebugs commented Mar 3, 2022

andydotxyz commented Mar 3, 2022

andydotxyz commented Mar 21, 2022

Jacalz commented Mar 21, 2022 •

edited

Loading

andydotxyz commented Mar 22, 2022

andydotxyz commented Mar 22, 2022

Jacalz commented Mar 22, 2022

andydotxyz commented Mar 30, 2022

Jacalz left a comment

andydotxyz commented Apr 1, 2022

Feature/metadataapi #2813

Feature/metadataapi #2813

Conversation

andydotxyz commented Feb 28, 2022 • edited Loading

Checklist:

lucor left a comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

coveralls commented Mar 1, 2022 • edited Loading

andydotxyz commented Mar 1, 2022 • edited Loading

Bluebugs commented Mar 1, 2022

andydotxyz commented Mar 1, 2022

andydotxyz commented Mar 1, 2022

Jacalz commented Mar 2, 2022

Bluebugs commented Mar 2, 2022

Jacalz commented Mar 3, 2022

andydotxyz commented Mar 3, 2022

Bluebugs commented Mar 3, 2022

Bluebugs commented Mar 3, 2022

andydotxyz commented Mar 3, 2022

andydotxyz commented Mar 21, 2022

Jacalz commented Mar 21, 2022 • edited Loading

andydotxyz commented Mar 22, 2022

andydotxyz commented Mar 22, 2022

Jacalz commented Mar 22, 2022

andydotxyz commented Mar 30, 2022

Jacalz left a comment

Choose a reason for hiding this comment

andydotxyz commented Apr 1, 2022

andydotxyz commented Feb 28, 2022 •

edited

Loading

coveralls commented Mar 1, 2022 •

edited

Loading

andydotxyz commented Mar 1, 2022 •

edited

Loading

Jacalz commented Mar 21, 2022 •

edited

Loading