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

add net.sourceforge.qtpfsgui.LuminanceHDR #715

Closed

Conversation

fcomida
Copy link

@fcomida fcomida commented Nov 11, 2018

Pull request for adding Luminance HDR to FlatHub

rename-appdata-file: luminance-hdr.appdata.xml
rename-icon: luminance-hdr
finish-args:
- "--socket=fallback-x11"
Copy link
Member

Choose a reason for hiding this comment

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

You don't need quotes here for strings

commit: e7ffd83af29187190da7f98dcbca8a4d70e19582

- name: fftw3
buildsystem: autotools
Copy link
Member

Choose a reason for hiding this comment

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

Not needed, it's the default buildsystem.

Choose a reason for hiding this comment

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

IMHO, it's better to state it explicitly, just in case default will change as well as others could understand what is going on easier (without the need to remember what are the defaults).

Copy link
Member

Choose a reason for hiding this comment

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

Except, default won't change unless meson take over the control of all the buildsystems out there :p if you take a look at the manifest, sometimes they mention the buildsystem: autotools and sometimes they don't :)

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect the default to ever change, but its not a blocker or anything if prefered.

buildsystem: cmake
sources:
- type: git
url: "https://github.com/Exiv2/exiv2/"
Copy link
Member

Choose a reason for hiding this comment

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

The url tags don't require quotes too.

@TingPing TingPing changed the title add manifest add net.sourceforge.qtpfsgui.LuminanceHDR Nov 11, 2018
rename-appdata-file: luminance-hdr.appdata.xml
rename-icon: luminance-hdr
finish-args:
- "--socket=fallback-x11"
Copy link
Member

Choose a reason for hiding this comment

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

fallback-x11 only makes sense if you also have wayland.

Choose a reason for hiding this comment

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

Regarding this, as far as I've been told,

Qt5 has a wayland backend so it can run on wayland

However neither @fcomida nor me running wayland. Thus we are not sure whether in modern distros

qt5 apps are run through wayland, and how flatpak interacts with wayland.

@TingPing can you help us to figure that out?

@TingPing
Copy link
Member

Are you involved with upstream or have you seem if they would be interested in being involved with this?

@fcomida
Copy link
Author

fcomida commented Nov 11, 2018

I am actually the main developer of Luminance HDR so yes, upstream is definitely involved.

Strings in yaml may not be quoted
cmake-ninja is more modern buildsystem with more features, why not use
it.
@TingPing
Copy link
Member

bot, build net.sourceforge.qtpfsgui.LuminanceHDR

@anxolerd
Copy link

@TingPing is everything covered in this PR? Or there are any changes we have to make in order to complete the package?

rename-appdata-file: luminance-hdr.appdata.xml
rename-icon: luminance-hdr
finish-args:
- --socket=fallback-x11
Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Thank you. Was not careful enough when reading that part of documentation

@bilelmoussaoui
Copy link
Member

@anxolerd By taking a look at the appdata and the desktop file, there are somethings that should be patched upstream.
For the appdata, the current file on the latest stable release

<?xml version="1.0" encoding="UTF-8"?>
<application>
  <id type="desktop">luminance-hdr.desktop</id>
  <metadata_license>CC0-1.0</metadata_license>
  <project-license>GPL-2.0+</project-license>
  <summary>Create HDR images</summary>
  <summary xml:lang="it">Crea immagini HDR</summary>
  <description>
    <p>Luminance HDR is an application for taking a set of pictures of the same scene with different exposure settings, and creating a high dynamic range (HDR) image. It supports a range on input images, including JPEG, TIFF and RAW. It can also rotate and crop, and tonemap HDR images.</p>
    <p xml:lang="it">Luminance HDR è un'applicazione per creare immagini ad elevato intervallo dinamico (HDR) a partire da un insieme di immagini di una stessa scena catturate con differente esposizione. Supporta svariati formati di input quali JPEG, TIFF e RAW. Può inoltre ruotare, ritagliare, scalare le immagini HDR e convertirle in formato a basso range dinamico (LDR) tramite tonemapping.</p>
  </description>
  <url type="homepage">http://qtpfsgui.sourceforge.net/</url>
  <screenshots>
    <screenshot type="default">http://qtpfsgui.sourceforge.net/screenshots/MainWindow-2.jpg</screenshot>
  </screenshots>
  <provides>
    <binary>luminance-hdr</binary>
  </provides>
  <provides>
    <binary>luminance-hdr-cli</binary>
  </provides>
</application>
  • The id should be replaced with net.sourceforge.qtpfsgui.LuminanceHDR without .desktop.
  • A launchable tag is missing
  • More urls could be used like Translation, Donation, Issues or Help.
  • OARS tag could be helpful too.
  • Releases tags are missing
  • The file should be renamed too.

The desktop file should be renamed too, the same for the icons. The Icon in the desktop file should be corrected to use the RDNN.

@bilelmoussaoui
Copy link
Member

Plus, you seem to be shipping the only 48px size of the icon, shipping other sizes would be nice! 128px is used on Flathub for example or you can just ship an SVG icon instead.

@bilelmoussaoui
Copy link
Member

You can also just run appstream-util upgrade on the file to get an upgraded version of the specs before editing anything. Once the modifications are done, you can use appstream-util validate-relax to validate the appdata file.

- add `--share=ipc` permission, as it is required when working with X11
  (for better performance)
- add `--share=network` permission as
  > there's a link on Help Browser pointing to the PayPal donation page
  > LHDR also checks for new versions on SF
Also Qt5 renders the UI via OpenGL if present and falls back to software rendering.
Add dri permission to fasten up renders using OpenGL
@anxolerd
Copy link

@bilelmoussaoui do I get it right that changes you suggest in #715 (comment) should be also done in the original app distribution as well?

@nedrichards
Copy link
Member

bot, build net.sourceforge.qtpfsgui.LuminanceHDR

As of flathub#715 (comment)
provide replacements for appdata.xml, .desktop files and icons to
comply.
@anxolerd
Copy link

@bilelmoussaoui I've tried to address your comment in aef90f5 .

@fcomida please check whether I stated all the information about LHDR(LuminanceHDR) correctly.

@fcomida
Copy link
Author

fcomida commented Nov 13, 2018

I integrated all changes in appdata file upstream, thank you.

@nedrichards
Copy link
Member

bot, build net.sourceforge.qtpfsgui.LuminanceHDR

net.sourceforge.qtpfsgui.LuminanceHDR.desktop Show resolved Hide resolved
net.sourceforge.qtpfsgui.LuminanceHDR.svg Show resolved Hide resolved
- --filesystem=home
- --share=network
- --device=dri
modules:
Copy link
Member

Choose a reason for hiding this comment

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

I suspect everything could use some cleanups of development headers etc.

Choose a reason for hiding this comment

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

@bilelmoussaoui regarding this, is flatpak-builder run... sufficient to look what's inside the built container? Or there is better option for this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, or you can inspect builddir/files directory.

Copy link
Member

Choose a reason for hiding this comment

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

What @barthalion suggested is perfect for this. A huge + if you can split the cleanup per module and put the common ones on the top cleanup, it's not a requirement, just a suggestion :)

@barthalion
Copy link
Member

In general only cleanups are a blocker, otherwise looks good to me.

Delete files which were required for build only and are not needed for
final distribution
@anxolerd
Copy link

@barthalion I've added cleanups as requested. Sorry for the delay.

@nedrichards
Copy link
Member

bot, build net.sourceforge.qtpfsgui.LuminanceHDR

post-install:
- install -Dm 644 net.sourceforge.qtpfsgui.LuminanceHDR.appdata.xml -t /app/share/appdata
- install -Dm 644 net.sourceforge.qtpfsgui.LuminanceHDR.desktop -t /app/share/applications
- install -Dm 644 net.sourceforge.qtpfsgui.LuminanceHDR.svg -t /app/share/icons/hicolor/scalable/apps
Copy link
Member

Choose a reason for hiding this comment

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

You might want to refresh the icon cache here, otherwise the icon might not show up

nedrichards pushed a commit to flathub/net.sourceforge.qtpfsgui.LuminanceHDR that referenced this pull request Nov 14, 2018
As of flathub/flathub#715 (comment)
provide replacements for appdata.xml, .desktop files and icons to
comply.
@nedrichards
Copy link
Member

Repository has been created: https://github.com/flathub/net.sourceforge.qtpfsgui.LuminanceHDR

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.

None yet

6 participants