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

Create io.github.xchan14.larawan.json #505

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Conversation

xchan14
Copy link
Contributor

@xchan14 xchan14 commented Feb 20, 2024

Review Checklist

  • App opens
  • Does what it says
  • Categories match

AppData

  • Name is unique and non-confusing
  • Matches description
  • Matches screenshot
  • Launchable tag with matching ID
  • Release tag with matching version and YYYY-MM-DD date
  • Custom colors meet WCAG A contrast or greater
  • OARS info matches

Flatpak

  • Uses elementary runtime
  • Sandbox permissions are reasonable

@xchan14 xchan14 requested a review from a team as a code owner February 20, 2024 20:32
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

Hey thanks for your submission! I haven't done a super thorough review yet, but on first glance there's a few things that need fixing before we can publish:

  • Your metainfo file contains placeholder text instead of a description
  • Screenshots show your entire desktop, they should only show your app
  • You app's description in metainfo and in your desktop file should not mention elementary or elementary OS
  • Your metainfo doesn't include any developer information such as your name and where folks can report issues or get help
  • I would highly encourage you not to add home folder permissions to your flatpak manifest and instead use the FileChooser portal to access files. Your app will be badged with a warning that it accesses the home folder if you add permissions in this way
  • There is a 7.3 runtime available, I would recommend updating to that :)

See https://docs.elementary.io/develop/appcenter/publishing-requirements for some more specific guidance

Also just a little tip, I can't remember if it's the 7.2 or 7.3 runtime, but Granite will now automatically load a gresourced stylesheet named "Application.css" (capitalization matters) when you run Granite.init, so there's no need to do that manually as in https://github.com/xchan14/larawan/blob/38cdb432c349c6309fff104b8074b91a219316ea/src/Application.vala#L31

Publishing requirements updates
@xchan14 xchan14 requested a review from danirabbit February 22, 2024 17:54
@danirabbit
Copy link
Member

Hey @xchan14 thanks for the quick turnaround! Everything is looking really good except it looks like there is no release tag in your metainfo file. Apologies for not catching that in the initial review. See the docs here for more information about creating a release tag: https://docs.elementary.io/develop/writing-apps/our-first-app/metadata#releases

Otherwise this seems okay to publish once we have that release tag! Congrats 🎉

A couple things I noticed here that aren't requirements but just advice:

You should init Granite and apply styles in startup, not in activate. Activate runs every time your app is launched from the dock or applications menu, including when folks click on the icon to switch windows. You really only want to do this once during your app's lifecycle, which is when the startup function runs.

There are built in icons that you can use with your app instead of using an emoji. There's an app in AppCenter called IconBrowser that you can use to view all the available system icons and get a code snippet. In this case I would recommend using Gtk.Button.from_icon_name ("open-menu"). Don't forget you should add a tooltip to image buttons so that there's something for the screen reader to read out to describe them

@xchan14
Copy link
Contributor Author

xchan14 commented Feb 24, 2024

Hi @danirabbit, done adding the release tag. As well as your advice. Thank you!

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

Looks good, nice work!

Just a couple things for your next release:

There's actually a method startup in Gtk.Application that you can override just like you override activate. So you want to do this instead of adding to your mainloop:

    public override void startup () {
        base.startup ();
        Granite.init ();
    }

And so you don't have to reach in and cast children inside of a button like ((Image) settings_button.child).pixel_size = 32; you can do settings_button.add_css_class (Granite.STYLE_CLASS_LARGE_ICONS) and that should be the same effect

There's also a portal you can use to request autostart for your app. It looks like we don't have it properly documented yet, but there's an issue report with a relevant code snippet and some other links: elementary/docs#159

@danirabbit danirabbit merged commit c0168af into elementary:main Feb 27, 2024
4 checks passed
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.

2 participants