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 ./configure --enable-fontforge-desktop #3387

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@JoesCat
Copy link
Contributor

JoesCat commented Dec 24, 2018

Add Freedesktop desktop support files and also merge-in added
modifications from Tobias Mueller muelli@cryptobitch.de
Support starts from older v1.0 upto what appears currently in
/usr/share/{appdata,applications,icons,mime,metainfo,pixmaps}
Theme support also added plus additional images included for
pixmaps and 2012 themes. Renamed directories from icons-old and
icons to 2012 and tango (allowing for further theme expansion).

Closes issue #3372, #3056, #3053, #892, #3368

@copyninja, @pnemade, @muelli - please check.

FYI, my internet is down, so I won't be able to reply quickly.
Add any patching you think necessary.
Thanks.

Add ./configure --enable-fontforge-desktop
Add Freedesktop desktop support files and also merge-in added
modifications from Tobias Mueller <muelli@cryptobitch.de>
Support starts from older v1.0 upto what appears currently in
/usr/share/{appdata,applications,icons,mime,metainfo,pixmaps}
Theme support also added plus additional images included for
pixmaps and 2012 themes. Renamed directories from icons-old and
icons to 2012 and tango (allowing for further theme expansion).

@JoesCat JoesCat requested review from frank-trampe and jtanx Dec 24, 2018

@frank-trampe

This comment has been minimized.

Copy link
Contributor

frank-trampe commented Dec 26, 2018

I don't really understand what's happening, but I'm satisfied that this is safe to merge.

@frank-trampe

This comment has been minimized.

Copy link
Contributor

frank-trampe commented Dec 26, 2018

Other stakeholders please comment promptly, as I would really like to tag a release before the end of the year.

@JoesCat

This comment has been minimized.

Copy link
Contributor

JoesCat commented Dec 26, 2018

Almost ok.... had too many arguments for AM_CONDITIONAL (4 is too many)....got a fix here by putting decision in an if/then decision, but internet problems are causing havoc.... hope to get replacement modem in a couple days.

AM_CONDITIONAL to if/then
AM_CONDITIONAL complained of too many arguments.

--------------------------------- configure.ac ---------------------------------
index ace319b..69549f8 100644
@@ -686,9 +686,13 @@ AC_PATH_PROG([DESKTOP_FILE_INSTALL],[desktop-file-install],[:])
AC_PATH_PROG([UPDATE_MIME_DATABASE],[update-mime-database],[:])
AC_PATH_PROG([UPDATE_DESKTOP_DATABASE],[update-desktop-database],[:])

-AM_CONDITIONAL([XDG_DESKTOP],

  • [test x"${fontforge_desktop}" != xno -a test x"${DESKTOP_FILE_INSTALL}" != "x:"
  • -a test x"${UPDATE_MIME_DATABASE}" != "x:" -a test x"${UPDATE_DESKTOP_DATABASE}" != "x:"])
    +i_have_desktop=yes
    +if test x"${fontforge_desktop}" = xno || test x"${DESKTOP_FILE_INSTALL}" = "x:"; then
  • i_have_desktop=no
    +if test x"${UPDATE_MIME_DATABASE}" = "x:" || test x"${UPDATE_DESKTOP_DATABASE}" = "x:"; then
  • i_have_desktop=no
    +fi
    +AM_CONDITIONAL([XDG_DESKTOP],[test x"${i_have_desktopUPDATE_MIME_DATABASE}" = xyes])

#--------------------------------------------------------------------------

Create and Set FontForge Compiler Flags

endif

desktopdir = $(PACKAGEROOTDIR)/applications
desktop_DATA = fontforge.desktop

This comment has been minimized.

@muelli

muelli Dec 28, 2018

Contributor

Have you not changed the appid on purpose?
Otherwise these files, i.e. the desktop, appdata, and icon files, need to be renamed to resemble the appid, i.e. org.fontforge.FontForge.

This comment has been minimized.

@JoesCat

JoesCat Dec 29, 2018

Contributor

I'll need to review this in better detail when I get propper internet again.
This patch attempts to try meet adding a desktop (requested), and also attempt to maintain older ver1 information, but also try and include newer data in the metainfo directory to meet latest version requirements.
Do you think making a duplicate of fontforge.metainfo.xml in /usr{/local}/share/appdata could be helpful? I did see a couple metainfo.xml in the appdata directory too.

</screenshots>
<url type="homepage">https://github.com/fontforge/fontforge</url>
<update_contact>fontforge-devel@lists.sourceforge.net</update_contact>
<content_rating type="oars-1.1"/>

This comment has been minimized.

@muelli

muelli Dec 28, 2018

Contributor

What's the purpose of removing OARS and release history and so on?

This comment has been minimized.

@JoesCat

JoesCat Dec 29, 2018

Contributor

Duplicated fontforge.appdata.xml to fontforge.metainfo.xml and then modified with your edits.
fontforge.metainfo.xml then gets loaded into /usr{/local)/share/metainfo while fontforge.appdata.xml gets entered into the existing /usr{/local}/share/appdata

I tried to be a little conservative in keeping appdata more in terms of ver1.0, while metainfo includes the more updated info you are applying. We also need to keep in mind another patch of interest concerning building an application meeting the older specs for ubuntu

When I looked at various examples in mageia5,6,7 and fedora24, it was a challenge to find anything other than org.kde and one example of org.gnome

is /usr{/local}/share/metainfo/fontforge.metainfo.xml okay?

JoesCat added some commits Dec 29, 2018

@JoesCat

This comment has been minimized.

Copy link
Contributor

JoesCat commented Dec 29, 2018

@frank-trampe - This can be squash-merge when you're ready to go, but prefer to also see feedback.

if/when --enable-fontforge-desktop it will add desktop support files in /usr{/local}/share/{appdata,applications,pixmaps,icons,metainfo,mime,mime/packages}.
if/when --enable-fontforge-desktop=PATH, then it will apply files in location PATH instead.
If DESTDIR is empty, then assumption made that this is for user PC and not some other location.

Patch is based on looking at a whole bunch of different applications pushing files into the /usr/share area, plus what we need for FontForge, including tango, 2012, and potentially other themes.

@JoesCat

This comment has been minimized.

Copy link
Contributor

JoesCat commented Dec 30, 2018

@muelli - a quick check gives the impression we're looking at two different specs....
1 - desktop: https://specifications.freedesktop.org/desktop-entry-spec/1.2/ar01s02.html
2 - appstream: https://www.freedesktop.org/software/appstream/docs/
...the desktop spec (1) seems ok with maintaining "fontforge.desktop" in ver1.2
...I'll read-thru (2) since existing /usr directory examples I looked at didn't appear to show reverse dns naming except for (mainly) org.kde and one or two org.gnome files.

@muelli

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment