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 various fixes #32

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add various fixes #32

wants to merge 7 commits into from

Conversation

scx
Copy link

@scx scx commented Feb 8, 2019

This is related to submitting my flatpak package to the Flathub repository.
flathub/flathub#847
https://github.com/scx/flathub/tree/com.github.wjaguar.mtPaint
https://github.com/scx/mtpaint-flatpak
#31

1. Replace png_set_gray_1_2_4_to_8 with png_set_expand_gray_1_2_4_to_8

scx@c1dd14d

The function png_set_gray_1_2_4_to_8() was removed from libpng. It has been deprecated since libpng-1.0.18 and 1.2.9, when it was replaced with png_set_expand_gray_1_2_4_to_8() because the former function also expanded palette images.

https://libpng.sourceforge.io/ANNOUNCE-1.4.0.txt

# grep -HER 'png_set_(expand_)?gray_1_2_4_to_8' /usr/include/libpng*/
/usr/include/libpng15/png.h:PNG_EXPORT(27, void, png_set_expand_gray_1_2_4_to_8, (png_structp png_ptr));

Equivalent in my flatpak package:
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtPaint.yaml#L53-L60
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtpaint-3.31-png.patch

See also:
https://src.fedoraproject.org/rpms/mtpaint/blob/f175c6a6b4e56bca4f053e608200c7023d33ccef/f/mtpaint.spec#_16
https://src.fedoraproject.org/rpms/mtpaint/blob/f175c6a6b4e56bca4f053e608200c7023d33ccef/f/mtpaint-3.31-png.patch

2. Include <locale.h> to avoid error: 'LC_ALL' undeclared

scx@e7e14dd

LC_ALL needs locale.h.

http://repo-test.flathub.org:8010/#/builders/6/builds/1915
http://repo-test.flathub.org:8010/api/v2/logs/37486/raw

mainwindow.c:5654:12: error: ‘LC_ALL’ undeclared (first use in this function)
  setlocale(LC_ALL, txt);

Equivalent in my flatpak package:
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtPaint.yaml#L64-L66
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtpaint-3.49.13-include-locale.patch#L8

See also:
openzfs/zfs@92e91da
https://bugs.launchpad.net/lightdm-gtk-greeter/+bug/999438

3. Replace firefox by xdg-open

scx@e5b461c

xdg-open opens a file or URL in the user's preferred application. If a URL is provided the URL will be opened in the user's preferred web browser. If a file is provided the file will be opened in the preferred application for files of that type. xdg-open supports file, ftp, http and https URLs.

Equivalent in my flatpak package:
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtPaint.yaml#L67-L70
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtpaint-3.49.13-spawn-doc.patch#L11

See also:
https://src.fedoraproject.org/rpms/mtpaint/blob/f175c6a6b4e56bca4f053e608200c7023d33ccef/f/mtpaint.spec#_15
https://src.fedoraproject.org/rpms/mtpaint/blob/f175c6a6b4e56bca4f053e608200c7023d33ccef/f/mtpaint-3.40-xdg-open.patch

4. Replace kprinter by yad

scx@7e88feb

kprinter is part of kdebase3, so it's really outdated now.
https://bugzilla.redhat.com/show_bug.cgi?id=964588

In KDE 3 KPrinter was responsible for printing of KDE applications, but other programs used it as well: if they had no own printing configuration but the possibility to add a generic command (like lp/lpr) they were often configured to print against the KPrinter command. KPrinter took the printed file and provided the the user a modern and flexible graphical user interface dialog to pick the preferred printer, change the printer configuration and so on.

With the transition to KDE 4 KPrinter vanished in favor of the Qt print dialog options, which worked only for Qt/KDE programs. All other programs outside Qt/KDE which relied on KPrinter as a drop-in command line tool were at a loss.
https://liquidat.wordpress.com/2014/02/18/kprinter-available-for-kde-4/

Although there is KPrinter4 for KDE 4, it has not gained much popularity. What is worse, its development has stuck in 2014.
https://github.com/credativ/kprinter4

Equivalent in my flatpak package:
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtPaint.yaml#L71-L74
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtpaint-3.49.13-spawn-commands.patch#L18

See also:
https://src.fedoraproject.org/rpms/mtpaint/blob/f175c6a6b4e56bca4f053e608200c7023d33ccef/f/mtpaint.spec#_18
https://src.fedoraproject.org/rpms/mtpaint/blob/f175c6a6b4e56bca4f053e608200c7023d33ccef/f/mtpaint-3.40-yad.patch
https://bugzilla.redhat.com/show_bug.cgi?id=964588#c10

5. Update desktop file

scx@464a315

Add StartupWMClass entry.

If specified, it is known that the application will map at least one window with the given string as its WM class or WM name hint (see the Startup Notification Protocol Specification for more details).

Equivalent in my flatpak package:
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtPaint.yaml#L82-L85

See also:
https://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#id1341
https://www.freedesktop.org/wiki/Specifications/startup-notification-spec

6. Add AppData file

scx@f0d2398

Register as an application to be visible in the software center.

You should also consider changing application id, as well as renaming AppData, desktop and icon file.
https://github.com/flathub/flathub/wiki/AppData-Guidelines#id
Although It is not required, it is highly recommended.

Equivalent in my flatpak package:
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtPaint.yaml#L50-L52
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtpaint.appdata.xml

See also:
https://src.fedoraproject.org/rpms/mtpaint/blob/f175c6a6b4e56bca4f053e608200c7023d33ccef/f/mtpaint.spec#_88
https://github.com/flathub/flathub/wiki/AppData-Guidelines
https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-launchable
https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-releases
https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-provides
https://hughsie.github.io/oars

7. Update Makefile: Install AppData file

scx@e20b84e

Install AppData file in the right directory.

Equivalent in my flatpak package:
https://github.com/scx/mtpaint-flatpak/blob/bbbb459ab768b80b5edf4b28c4b3543979cc93f4/mtPaint.yaml#L100-L101

See also:
https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#spec-component-location
flatpak/flatpak#30
flatpak/flatpak-builder#252 (comment)

@wjaguar
Copy link
Owner

wjaguar commented Feb 8, 2019

  1. Replace png_set_gray_1_2_4_to_8 with png_set_expand_gray_1_2_4_to_8

Please do read actual code in png.c , particularly the lines 618-620.
Overlooking stuff like this is bad for a coder. :(

  1. Include <locale.h> to avoid error: 'LC_ALL' undeclared

Thanks. Actually it should be there for setlocale() itself, too.
Even if no real system anywhere ever needed it included explicitly. :)

  1. Replace firefox by xdg-open

Agreed. Especially given the present state of firefox :)

  1. Replace kprinter by yad

Neither present in default install of Slackware. Anyhow it's more an example than anything, as the print action is just as user-configurable as any other. Any distro-specific preconfiguration is expected to be done by packager through /etc/mtpaint/mtpaintrc:
http://mtpaint.sourceforge.net/handbook/en_GB/chap_A.html#SEC64

  1. Update desktop file
    Add StartupWMClass entry.

3 such things in all of Slackware, aka perfect example of "Not Needed Ever". Let some other guys be early adopters.

  1. Add AppData file
  2. Update Makefile: Install AppData file

50 such files in the entire Slackware install, does not look like accepted standard even with a microscope. :)

@scx
Copy link
Author

scx commented Feb 8, 2019

First of all, thank you for the quick reply.

  1. Replace png_set_gray_1_2_4_to_8 with png_set_expand_gray_1_2_4_to_8

Please do read actual code in png.c , particularly the lines 618-620.
Overlooking stuff like this is bad for a coder. :(

Sorry, my mistake. I am sure that it was needed in mtPaint 3.31, then I saw that the current version still uses png_set_gray_1_2_4_to_8(), so I through that this patch is still required. Anyway, my bad.

  1. Replace kprinter by yad

Neither present in default install of Slackware. Anyhow it's more an example than anything, as the print action is just as user-configurable as any other. Any distro-specific preconfiguration is expected to be done by packager through /etc/mtpaint/mtpaintrc:
http://mtpaint.sourceforge.net/handbook/en_GB/chap_A.html#SEC64

Well, I am just saying that kprinter is quite obsoleted now. It's just hard to get it in modern distro, even if we are talking about the Qt4 version. I don't insist on yad. It is more like a suggestion that maybe it would be worth inserting a more modern program here. As far as I know, Debian uses gtklp for this purpose.
https://pkgs.org/download/yad
https://pkgs.org/download/gtklp
https://pkgs.org/download/kprinter

  1. Update desktop file
    Add StartupWMClass entry.

3 such things in all of Slackware, aka perfect example of "Not Needed Ever". Let some other guys be early adopters.

Even in RHEL 7, there are quite a lot of applications that use it, for example LibreOffice Suite (Calc, Draw, Math, Writer, Impress), WPS Office, Chromium browser, Visual Studio Code, Emacs, XEmacs, Gajim, GeoGebra, GNOME Tweaks, Java PolicyTool, qBittorrent, Scribus, X2Go Client, etc. It is even more popular in modern distributions like Fedora.

  1. Add AppData file
  2. Update Makefile: Install AppData file

50 such files in the entire Slackware install, does not look like accepted standard even with a microscope. :)

There are already over half thousand applications on Flathub. Currently, 493 programs are highlighted on the flathub.org website, so it's easy to find out about them. All must have an AppData file, because it is required by Flathub. Still not enough? Well, we have more than one thousand packages in Fedora 29, that provide at least one AppData file (upstream or downstream).
https://pastebin.ubuntu.com/p/DZ5BhFcCX8/

I am not going to force you to put the AppData into this repo. However, it is de facto standard for any modern Linux GUI app, and it is easier for everyone when it is upstreamed, so every distribution can use the same file instead of creating its own version.

Anyway, this pull request is mainly about the AppData file. There's not much sense to continue without it. I mean, it would be easier if you make the two other changes (2. and 3.) by yourself, right?

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