Comment 1 Yuri Victorovich 2018-02-23 00:07:13 UTC

(In reply to lightside from comment #0)

INSTALLS_ICONS=yes is only required when GTK is in use, which isn't the case here.

Comment 2 Yuri Victorovich 2018-02-23 00:08:56 UTC

Committed, thanks!

Comment 3 commit-hook 2018-02-23 00:09:35 UTC

A commit references this bug:

Author: yuri
Date: Fri Feb 23 00:08:53 UTC 2018
New revision: 462654
URL: https://svnweb.freebsd.org/changeset/ports/462654

Log:
  editors/focuswriter: Update 1.6.10

  PR:		226122
  Submitted by:	lightside <lightside@gmx.com> (maintainer)
  Approved by:	tcberner (mentor, implicit)

Changes:
  head/editors/focuswriter/Makefile
  head/editors/focuswriter/distinfo

Created attachment 190912 [details]
Proposed patch (since 462654 revision)

(In reply to comment #2)
> INSTALLS_ICONS=yes is only required when GTK is in use, which isn't the case
> here.
This is why I proposed optional CACHE_ICONS port's option with following description:
"Update icon cache after installation (GTK+)".

The CACHE_ICONS port's option is not enabled by default (i.e. no OPTIONS_DEFAULT=CACHE_ICONS), but can be enabled in case of usage of GTK+ applications (e.g. which may display high-color icons for *.desktop files).

The INSTALLS_ICONS=yes will add @postexec and @postunexec rules to generated pkg-plist, for example:
https://github.com/freebsd/freebsd-ports/blob/f94c0be9433c87594b2ddb224b838f07abed216d/Mk/Uses/gnome.mk#L535-L552

The current description for INSTALLS_ICONS is misleading (about QT based applications and icons, which used for misc/hicolor-icon-theme), in my opinion:
https://github.com/freebsd/freebsd-ports/blob/f94c0be9433c87594b2ddb224b838f07abed216d/Mk/Uses/gnome.mk#L52-L56
-8<--
# INSTALLS_ICONS	- If a GTK+ port installs Freedesktop-style icons to
#				${LOCALBASE}/share/icons, then you should use this
#				macro. Using this macro ensures that icons are cached
#				and will display correctly. This macro isn't needed
#				for QT based applications, which use a different method.
-->8-

Previous description for INSTALLS_ICONS macro has clear description of consequences of not using it:
https://github.com/freebsd/freebsd-ports/blob/3d272740920a1868e8acfed76d6e4c67c331e101/Mk/bsd.gnome.mk#L64-L67
-8<--
# INSTALLS_ICONS	- If your port installs Freedesktop-style icons to
#				${LOCALBASE}/share/icons, then you should use this
#				macro. If the icons are not cached, they will not be
# displayed.
-->8-

Please commit full patch.

I requested portmgr@ maintainer approval, because yuri@ is an interested person, who proposed changes for ports-mgmt/portlint in bug #223498. There is a need for neutral opinion about this proposal.

Re-open.

Comment 6 Yuri Victorovich 2018-02-23 02:51:33 UTC

(In reply to lightside from comment #4)

Either all ports should do this, or none.
One port can't do this individually.
If this should ever be done (which I don't think it should), this should be done in Mk, not here.

Comment 7 Yuri Victorovich 2018-02-23 02:55:57 UTC

lightside,

FYI All commits are reviewed by other committers. It's not like I do something outrageous. What you request is out of the ordinary, and isn't what should be done here.

We had this discussion before, I remember this.

Cheers!
Yuri

(In reply to Yuri Victorovich from comment #7)
> We had this discussion before, I remember this.
We had no discussions about this. I preventively wrote explanation in bug 225770 comment #1 about the need of INSTALLS_ICONS=yes with testcase for x11-fm/caja. The w.schwarzenfeld wrote own opinions in bug 225770 comment #2 and bug 225770 comment #4. Then you did ports r461416 changes, which removed INSTALLS_ICONS=yes and said about testcase after this in bug 225770 comment #7.

I wait for opinion and maintainer-approval from portmgr@. Please re-open and re-assign to freebsd-ports-bugs@. Thanks for commit, related to port's update.

CC: w.schwarzenfeld@

Comment 9 Yuri Victorovich 2018-02-23 03:43:51 UTC

(In reply to lightside from comment #8)

There are hundreds of ports just like that.
If you want this change, please open a separate bug report, create a review on https://reviews.freebsd.org/, add portmgr as a reviewer, and discuss this with portmgr.

This has nothing to do with this bug #226122 which is "Update to 1.6.10".

This bug is closed.

Comment 10 Yuri Victorovich 2018-02-23 03:46:55 UTC

Committers are here to check submitted updates, and I stopped this because it is unreasonable.

I recommend you put more trust in what committers do, especially after they repeatedly expressed disagreement with your approach.

(In reply to Yuri Victorovich from comment #10)
> I recommend you put more trust in what committers do, especially after they
> repeatedly expressed disagreement with your approach.
I recommend to not make this as personal. Possible, that there is some misunderstanding here.

The possible cause are different approaches used by KDE and Gnome applications.
In case of KDE, there is some dynamic approach to cache icons, when they are needed:
https://userbase.kde.org/KDE_System_Administration/Caches

In case of Gnome, there is some usage of gtk-update-icons-cache application to update ${LOCALBASE}/share/icons/hicolor/icon-theme.cache file after installation (and deinstallation) of package.

I may understand some desire of KDE users to not install graphics/gtk-update-icon-cache dependency, which added by current INSTALLS_ICONS macro:
https://github.com/freebsd/freebsd-ports/blob/f94c0be9433c87594b2ddb224b838f07abed216d/Mk/Uses/gnome.mk#L391-L393

But why GTK+ users need to be ignored?

There is a clear alternative to add CACHE_ICONS option (or with different name, if needed), which may use INSTALLS_ICONS macro. So, KDE's users may not install graphics/gtk-update-icon-cache dependency, when needed, while GTK+ related users may use correct high-color icons (for example, ${LOCALBASE}/share/icons/hicolor/scalable/apps/focuswriter.svg, instead of low-resolution ${LOCALBASE}/share/pixmaps/focuswriter.xpm, to display ${LOCALBASE}/share/applications/focuswriter.desktop, in this case).

The ${LOCALBASE}/share/icons/hicolor/scalable/apps/focuswriter.svg is not directly related to QT application. It may be related to some desktop environment and directories by misc/hicolor-icon-theme port:
https://github.com/freebsd/freebsd-ports/blob/a8ab25c1f18556efbc554370e73b0d3dfc8fe2e4/misc/hicolor-icon-theme/pkg-plist#L316

Possible, that there are other methods to solve this issue with icon cache for GTK+ applications.

If this will be not resolved for some reasons, I may suggest to use following editors/focuswriter-cache-icons port privately for interested users:
editors/focuswriter-cache-icons/Makefile:
-8<--
# $FreeBSD$

PKGNAMESUFFIX=	-cache-icons

CONFLICTS_INSTALL=	${PORTNAME}-[0-9]*

OPTIONS_SLAVE=		CACHE_ICONS
CACHE_ICONS_DESC=	Update icon cache after installation (GTK+)
CACHE_ICONS_VARS=	INSTALLS_ICONS=yes

MASTERDIR=	${.CURDIR}/../focuswriter

.include "${MASTERDIR}/Makefile"

OPTIONS_DEFINE+=	CACHE_ICONS
-->8-

Comment 12 Yuri Victorovich 2018-02-24 01:22:45 UTC

lightside,

Here is a more detailed explanation.

This bug is about a version update (see subject). The version update is very simple, and is already committed.

You suggestion about INSTALLS_ICONS contradicts this line in Mk/:
> Mk/Uses/gnome.mk:# INSTALLS_ICONS	- If a GTK+ port installs Freedesktop-style icons to
We do not do such things in ports.

If you have a different opinion about the applicability of INSTALLS_ICONS, you should recognize that this doesn't have anything to do with this particular port.

If you would like to proceed, you shouldn't be insisting to add INSTALLS_ICONS with the unrelated port version update. Please open a separate review. Add portmgr@ as a reviewer there. portmgr@ is the right person to discuss this with.

portmgr@ is in charge of Mk/. Convince him that you are right, and he will help you there, and will change the rules. But it isn't reasonable to demand that such thing is just committed with a version update.


Cheers!
Yuri

(In reply to Yuri Victorovich from comment #12)
Most of the arguments about INSTALLS_ICONS=yes and related CACHE_ICONS port's option was already explained in my previous comments.

I already understood, that you didn't fully commit (in ports r462654) what was proposed in this PR (comment #0) and related attachment #190898 [details], but wanted to participate with partial commit for some reason(s). I may understand that you can't (don't want to) commit other remained changes (attachment #190912 [details]) for some reason(s). It's fine.

On the other hand, I just asked (in comment #4) for neutral opinion from portmgr@ about attachment #190912 [details]. This is understandable, that some portmgr@ may ignore or don't check this PR. Anyway, there is some solution available in comment #11, in case some user may want to use high-color icons (for GTK+ environment), installed by editors/focuswriter port. I didn't ask to solve this for me. Some solution(s) already available.

Interested people, who may understand the cause of issues, related to icon cache, may check the need of INSTALLS_ICONS=yes (or gtk-update-icon-cache invocation after changes for icons in /usr/local/share/icons/* directories) by comparing checksums of /usr/local/share/icons/hicolor/icon-theme.cache before and after installation of editors/focuswriter port with using INSTALLS_ICONS=yes. If checksum will be the same, then INSTALLS_ICONS=yes is not needed.

My results shows, that checksum is different after installation, therefore INSTALLS_ICONS=yes is needed, in my case:
-8<--
% sha256 /usr/local/share/icons/hicolor/icon-theme.cache
SHA256 (/usr/local/share/icons/hicolor/icon-theme.cache) = b002b54c22b93e4bee67f64401c49696942ab92dfae7916305f7dde1f4f24d1c
# pkg add focuswriter-cache-icons-1.6.10.txz
<..>
% sha256 /usr/local/share/icons/hicolor/icon-theme.cache
SHA256 (/usr/local/share/icons/hicolor/icon-theme.cache) = 75f4e7bb9b1b3f49efab62a40b70dba2b835de0c1382a899ae50a832cfcbafaa
# pkg delete focuswriter-cache-icons
<..>
% sha256 /usr/local/share/icons/hicolor/icon-theme.cache
SHA256 (/usr/local/share/icons/hicolor/icon-theme.cache) = b002b54c22b93e4bee67f64401c49696942ab92dfae7916305f7dde1f4f24d1c
-->8-

Other method is to check what displayed for /usr/local/share/applications/focuswriter.desktop with/without using of INSTALLS_ICONS=yes for editors/focuswriter port, in case of some GTK+ environment (e.g. by using caja (from x11-fm/caja port) to open /usr/local/share/applications/ directory and selecting focuswriter.desktop file).

Comment 14 Walter Schwarzenfeld 2018-02-24 07:08:29 UTC

Reopen - make discussion "public".

Comment 15 Walter Schwarzenfeld 2018-02-24 07:13:07 UTC

I don't want to decide it. Yuri, I know you will 100% correct, but I don't really understand where is the problem to make an option? If you grep for INSTALLS_ICONS through the ports-tree you can find some qt-ports with it (qupzilla e.g.).

Comment 16 Walter Schwarzenfeld 2018-02-24 07:17:27 UTC

corr: want to be 100% correct (instead of will).

Comment 17 Kubilay Kocak 2018-02-24 07:43:58 UTC

Request portmgr (& gnome team) advice/decision on:

- INSTALL_ICONS being allowed by individual ports, without (or until) current framework standardisation (I am not aware of the current state)
- In what manner INSTALL_ICONS should be used, and what considerations should be made (for example, with regard to conditional/OPTION'al inclusion).

Note: The above questions are separate to the question of 'to what extent changes should be combined or separated into distinct issues (patches)' While there is a defacto lean towards isolated changes, the reality is it is not a consistently (if at all) enforced standard, nor has objective or documented measures.

Participants are reminded that issue tracker content/comments become permanent and public record, and should reflect the very best of peoples abilities to remain objective, factual and considerate.

Comment 18 Kubilay Kocak 2018-02-24 07:44:15 UTC

Comment on attachment 190898 [details]
Proposed patch (since 461416 revision)

Older revision

Comment 19 Kubilay Kocak 2018-02-24 07:45:18 UTC

Comment on attachment 190898 [details]
Proposed patch (since 461416 revision)

Not an obsoleted patch.

Comment on attachment 190898 [details]
Proposed patch (since 461416 revision)

(In reply to Kubilay Kocak from comment #19)
> Not an obsoleted patch.
Ok, changed status of attachment #190898 [details] to obsolete.
Thanks for your participation.

Comment 21 Kubilay Kocak 2018-02-24 09:10:40 UTC

Comment on attachment 190898 [details]
Proposed patch (since 461416 revision)

Its better if this original patch remains visible, as it was the patch used (incompletely) for the original commit.

Created attachment 191129 [details]
Proposed patch (without INSTALLS_ICONS, since 462654 revision)

If there is some misunderstanding about INSTALLS_ICONS macro, because of some (misleading) description (see comment #4) in Mk/Uses/gnome.mk, then possible to add needed commands on post-install stage, which does the same. I attached new patch for this, which approved by current maintainer, who may understand why this is needed.

Created attachment 191132 [details]
Proposed patch (without INSTALLS_ICONS, since 462654 revision)

(In reply to comment #22)
> then possible to add needed commands on post-install stage,
> which does the same.
The editors/libreoffice port does the same for needed share/icons subdirectories, for example:
https://github.com/freebsd/freebsd-ports/blob/51047017b694d5de31ca9f5d3d0eb760e55ccd88/editors/libreoffice/Makefile#L325-L330
-8<--
add-plist-gnome:
.for subdir in gnome hicolor locolor
	@${ECHO_CMD} "@rmtry share/icons/${subdir}/icon-theme.cache" >> ${TMPPLIST}
	@${ECHO_CMD} "@postexec ${LOCALBASE}/bin/gtk-update-icon-cache -q -f %D/share/icons/${subdir} 2>/dev/null || ${TRUE}" >> ${TMPPLIST}
	@${ECHO_CMD} "@postunexec ${LOCALBASE}/bin/gtk-update-icon-cache -q -f %D/share/icons/${subdir} 2>/dev/null || ${TRUE}" >> ${TMPPLIST}
.endfor
-->8-

Comment 24 Walter Schwarzenfeld 2019-02-10 23:17:56 UTC

Makefile shows:
DISTVERSION=    1.7.1
PORTREVISION=   2

close with overcome by events.

(In reply to comment #24)
> Makefile shows:
> DISTVERSION=    1.7.1
> PORTREVISION=   2
The patches in attachment #190912 [details] or attachment #191132 [details] doesn't mention DISTVERSION or PORTREVISION changes.

(In reply to comment #24)
> close with overcome by events.
The issue with cached icons (after (de)installation) is not resolved. See comment #13 for possible tests.

I tried to propose the same fixes for next version (see bug #227142), but this didn't work (i.e. returned to current PR) and probably requires objective attention from portmgr@, gnome@.

Reopen with clarified summary.

Comment 26 Tobias C. Berner 2020-03-08 17:08:42 UTC

Moin moin 

Please create a new bug for the icon handling. 
I close this one, as the "maintainer approved" upgrade part has been done.


mfg Tobias