mergetool-lib: Don't use deprecated variable to detect GNOME
cover mergetool-lib: Don't use deprecated variable to detect GNOME
To list merge tool candidates we used to use a private GNOME env variable ( GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and removed as part of GNOME 3.30.0 release [1].
So replace this using XDG_CURRENT_DESKTOP instead, and cleanup the code to avoid duplication and supporting KDE's kdiff3 better.
[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65
Marco Trevisan (Treviño) (3):
mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME
mergetool-lib: keep a list of cross desktop merge tools
mergetool-lib: give kdiff3 prioirty in KDE environments
git-mergetool--lib.sh | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)
base-commit: 85b4e0a6dc8407de6f69808d9ee6debdf167ced3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-693%2F3v1n0%2Fdesktop-envs-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-693/3v1n0/desktop-envs-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/693
Signature · 1 line
gitgitgadget
PATCH 1/3 mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME
From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <[email protected]>
To list merge tool candidates we used to use a private GNOME env variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and removed as part of GNOME 3.30.0 release [1].
So, git should instead check the XDG_CURRENT_DESKTOP env variable, that is supported by all the desktop environments.
Since the variable is actually a colon-separated list of names that the current desktop is known as, we need to go through all the values to ensure we're using GNOME.
[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65
--- git-mergetool--lib.sh | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
git-mergetool--lib.sh +14 -1
@@ -266,6 +266,19 @@ run_merge_cmd () { fi } +is_desktop () { + IFS=':' + for desktop in ${XDG_CURRENT_DESKTOP} + do + if test "$desktop" = "$1" + then + return 0 + fi + done + + return 1 +} + list_merge_tool_candidates () { if merge_mode then
@@ -275,7 +288,7 @@ list_merge_tool_candidates () { fi if test -n "$DISPLAY" then - if test -n "$GNOME_DESKTOP_SESSION_ID" + if is_desktop "GNOME" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools" else
Signature · 1 line
gitgitgadget
PATCH 2/3 mergetool-lib: keep a list of cross desktop merge tools
In reply to Marco Trevisan via GitGitGadgetFrom: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <[email protected]>
Instead of repeating the same tools multiple times, let's just keep them in another variable and list them depending the current desktop
--- git-mergetool--lib.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
git-mergetool--lib.sh +3 -2
@@ -288,11 +288,12 @@ list_merge_tool_candidates () { fi if test -n "$DISPLAY" then + cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff" if is_desktop "GNOME" then - tools="meld opendiff kdiff3 tkdiff xxdiff $tools" + tools="meld $cross_desktop_tools $tools" else - tools="opendiff kdiff3 tkdiff xxdiff meld $tools" + tools="$cross_desktop_tools meld $tools" fi tools="$tools gvimdiff diffuse diffmerge ecmerge" tools="$tools p4merge araxis bc codecompare"
Signature · 1 line
gitgitgadget
PATCH 3/3 mergetool-lib: give kdiff3 prioirty in KDE environments
In reply to Marco Trevisan via GitGitGadgetFrom: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <[email protected]>
--- git-mergetool--lib.sh | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
git-mergetool--lib.sh +6 -3
@@ -288,12 +288,15 @@ list_merge_tool_candidates () { fi if test -n "$DISPLAY" then - cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff" + cross_desktop_tools="opendiff tkdiff xxdiff" if is_desktop "GNOME" then - tools="meld $cross_desktop_tools $tools" + tools="meld $cross_desktop_tools kdiff3 $tools" + elif is_desktop "KDE" + then + tools="kdiff3 $cross_desktop_tools meld $tools" else - tools="$cross_desktop_tools meld $tools" + tools="$cross_desktop_tools kdiff3 meld $tools" fi tools="$tools gvimdiff diffuse diffmerge ecmerge" tools="$tools p4merge araxis bc codecompare"
Signature · 1 line
gitgitgadget
Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME
In reply to Marco Trevisan (Treviño) via GitGitGadgetQuoted 15 lines · Marco Trevisan (Treviño) via GitGitGadget
To list merge tool candidates we used to use a private GNOME env variable (
GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and removed as part of GNOME 3.30.0 release [1].So, git should instead check the
XDG_CURRENT_DESKTOPenv variable, that is supported by all the desktop environments.Since the variable is actually a colon-separated list of names that the current desktop is known as, we need to go through all the values to ensure we're using GNOME.
Signed-off-by Marco Trevisan (Treviño) <[email protected]>---
git-mergetool--lib.sh run_merge_cmd () { · line 267
Quoted 2 lines
git-mergetool--lib.sh +2
-0@@ -266,6 +266,19 @@ run_merge_cmd () { +is_desktop () { + IFS=':'
We usually want to restore the value of IFS after we're done with it. For instance:
OLDIFS=$IFS
IFS=:and then restore it before returning:
IFS=$OLDIFS
git-mergetool--lib.sh run_merge_cmd () { · line 280
Quoted 10 lines
git-mergetool--lib.sh +10
-0+ for desktop in ${XDG_CURRENT_DESKTOP} + do + if test "$desktop" = "$1" + then + return 0 + fi + done + + return 1 +}
Rather than looping and mucking with IFS, even easier would be:
is_desktop () {
case ":$XDG_CURRENT_DESKTOP:" in
*:$1:*) return 0 ;;
*) return 1 ;;
esac
}But perhaps that's too magical for people?
git-mergetool--lib.sh list_merge_tool_candidates () { · line 288
Quoted 3 lines
git-mergetool--lib.sh +1
-1@@ -275,7 +288,7 @@ list_merge_tool_candidates () { - if test -n "$GNOME_DESKTOP_SESSION_ID" + if is_desktop "GNOME"
Why do we need to retire the $GNOME_DESKTOP_SESSION_ID check here, thus penalizing people who might still be on an old version of GNOME? It doesn't seem like it would be a maintenance burden to continue checking it while also taking advantage of $XDG_CURRENT_DESKTOP:
if test -n "$GNOME_DESKTOP_SESSION_ID" || is_desktop GNOME
Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools
In reply to Marco Trevisan (Treviño) via GitGitGadgetQuoted 8 lines · Marco Trevisan (Treviño) via GitGitGadget
Instead of repeating the same tools multiple times, let's just keep them in another variable and list them depending the current desktop
Signed-off-by Marco Trevisan (Treviño) <[email protected]>---
git-mergetool--lib.sh +0
-0@@ -288,11 +288,12 @@ list_merge_tool_candidates () { if test -n "$DISPLAY"
git-mergetool--lib.sh list_merge_tool_candidates () { · line 296
Quoted 10 lines
git-mergetool--lib.sh +3
-2then + cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff" if is_desktop "GNOME" then - tools="meld opendiff kdiff3 tkdiff xxdiff $tools" + tools="meld $cross_desktop_tools $tools" else - tools="opendiff kdiff3 tkdiff xxdiff meld $tools" + tools="$cross_desktop_tools meld $tools" fi
I have mixed feelings about this change. On the one hand, I see the reason for doing it if the list of tools remains substantially the same in each case, but it also seems like it could become a burden, possibly requiring factoring the list into more pieces, as new platforms or tools are added.
What I might find more compelling is creation of a table of tools and the platforms for which they are suitable. It doesn't seem like it would be too difficult to express such a table in shell and to extract the desired rows (but that might be overkill). At any rate, I'm rather "meh" on this change, though I don't oppose it strongly.
Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments
In reply to Marco Trevisan (Treviño) via GitGitGadgetQuoted 1 line · Marco Trevisan (Treviño) via GitGitGadget
mergetool-lib: give kdiff3 prioirty in KDE environments
s/prioirty/priority/
Quoted 7 lines
Signed-off-by Marco Trevisan (Treviño) <[email protected]>---
git-mergetool--lib.sh +1
-1@@ -288,12 +288,15 @@ list_merge_tool_candidates () { - cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff" + cross_desktop_tools="opendiff tkdiff xxdiff" if is_desktop "GNOME"
git-mergetool--lib.sh list_merge_tool_candidates () { · line 297
Quoted 10 lines
git-mergetool--lib.sh +5
-2then - tools="meld $cross_desktop_tools $tools" + tools="meld $cross_desktop_tools kdiff3 $tools" + elif is_desktop "KDE" + then + tools="kdiff3 $cross_desktop_tools meld $tools" else - tools="$cross_desktop_tools meld $tools" + tools="$cross_desktop_tools kdiff3 meld $tools" fi
Wouldn't this change the behavior for people running old KDE which doesn't have XDG_CURRENT_DESKTOP, giving "kdiff3" much lower priority than it had before?
This change also illustrates why I wasn't convinced that patch 2/3 was necessarily a good idea. When you touch 'cross_desktop_tools' here, you end up having to touch all the other 'tools=' lines anyhow, so the introduction of 'cross_desktop_tools' didn't buy much in terms of reduced maintenance cost.
Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME
In reply to Eric SunshineQuoted 10 lines · Eric Sunshine
Rather than looping and mucking with IFS, even easier would be:
is_desktop () { case ":$XDG_CURRENT_DESKTOP:" in *:$1:*) return 0 ;; *) return 1 ;; esac }But perhaps that's too magical for people?
Nah, that is exactly how 'case' in shell is supposed to be used.
Thanks.
Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools
In reply to Eric SunshineAm 05.08.20 um 23:08 schrieb Eric Sunshine:
Quoted 27 lines
Quoted 8 lines · Marco Trevisan (Treviño) via GitGitGadget
Instead of repeating the same tools multiple times, let's just keep them in another variable and list them depending the current desktop
Signed-off-by Marco Trevisan (Treviño) <[email protected]>---
git-mergetool--lib.sh +0
-0@@ -288,11 +288,12 @@ list_merge_tool_candidates () { if test -n "$DISPLAY"git-mergetool--lib.sh list_merge_tool_candidates () { · line 296
Quoted 10 lines
git-mergetool--lib.sh +3
-2then + cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff" if is_desktop "GNOME" then - tools="meld opendiff kdiff3 tkdiff xxdiff $tools" + tools="meld $cross_desktop_tools $tools" else - tools="opendiff kdiff3 tkdiff xxdiff meld $tools" + tools="$cross_desktop_tools meld $tools" fiI have mixed feelings about this change. On the one hand, I see the reason for doing it if the list of tools remains substantially the same in each case, but it also seems like it could become a burden, possibly requiring factoring the list into more pieces, as new platforms or tools are added.
Quoted 5 lines
What I might find more compelling is creation of a table of tools and the platforms for which they are suitable. It doesn't seem like it would be too difficult to express such a table in shell and to extract the desired rows (but that might be overkill). At any rate, I'm rather "meh" on this change, though I don't oppose it strongly.
There may be some confusion caused by the name of the new variable. A better name would perhaps be $tools_with_desktop_dependent_preference. (That's a tongue-in-cheek suggestion, BTW.)
On a side note, the refactoring that happened in 21d0ba7ebb0c ("difftool/mergetool: refactor commands to use git-mergetool--lib", 2009-04-08) did not preserve the original order of diff tools. The spirit was to prefer meld over kompare and kdiff3 with GNOME, and the other way round with KDE. But since that refactoring, kompare is always first in the list.
-- Hannes
Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments
In reply to Marco Trevisan (Treviño)Quoted 9 lines · Marco Trevisan (Treviño)
Il giorno mer 5 ago 2020 alle ore 23:16 Eric Sunshine <[email protected]> ha scritto:
Quoted 3 lines
Wouldn't this change the behavior for people running old KDE which doesn't have
XDG_CURRENT_DESKTOP, giving "kdiff3" much lower priority than it had before?Yeah, true.. So to avoid this we can just also check for
KDE_FULL_SESSION, that has been introduced by KDE 3.2, and this should be enough I think.
I'm not a user of git-mergetool or KDE, so I can't speak as an end-user. My comment was made merely as a reviewer of the code.
If it is easy to avoid the behavior change by also checking KDE_FULL_SESSION in addition to the new check of XDG_CURRENT_DESKTOP without it being a maintenance burden, then that sounds like a good choice. On the other hand, if there wasn't a good way to avoid changing behavior for users of older KDE, then explaining that in the commit message would be expected.
Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME
In reply to Eric SunshineHi,
Il giorno mer 5 ago 2020 alle ore 22:58 Eric Sunshine <[email protected]> ha scritto:
Quoted 24 lines
Quoted 15 lines · Marco Trevisan (Treviño) via GitGitGadget
To list merge tool candidates we used to use a private GNOME env variable (
GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and removed as part of GNOME 3.30.0 release [1].So, git should instead check the
XDG_CURRENT_DESKTOPenv variable, that is supported by all the desktop environments.Since the variable is actually a colon-separated list of names that the current desktop is known as, we need to go through all the values to ensure we're using GNOME.
Signed-off-by Marco Trevisan (Treviño) <[email protected]>---
git-mergetool--lib.sh run_merge_cmd () { · line 267
Quoted 2 lines
git-mergetool--lib.sh +2
-0@@ -266,6 +266,19 @@ run_merge_cmd () { +is_desktop () { + IFS=':'We usually want to restore the value of IFS after we're done with it. For instance:
Quoted 6 lines
OLDIFS=$IFS IFS=:and then restore it before returning:
IFS=$OLDIFS
Yeah, that's true and always a good practice, but it was never happening in this file, so I followed the style.
Quoted 13 lines
git-mergetool--lib.sh run_merge_cmd () { · line 280
Quoted 10 lines
git-mergetool--lib.sh +10
-0+ for desktop in ${XDG_CURRENT_DESKTOP} + do + if test "$desktop" = "$1" + then + return 0 + fi + done + + return 1 +}Rather than looping and mucking with IFS, even easier would be:
Quoted 8 lines
is_desktop () { case ":$XDG_CURRENT_DESKTOP:" in *:$1:*) return 0 ;; *) return 1 ;; esac }But perhaps that's too magical for people?
Ok, fair enough, this is fine as well.
Quoted 10 lines
git-mergetool--lib.sh list_merge_tool_candidates () { · line 288
Quoted 3 lines
git-mergetool--lib.sh +1
-1@@ -275,7 +288,7 @@ list_merge_tool_candidates () { - if test -n "$GNOME_DESKTOP_SESSION_ID" + if is_desktop "GNOME"Why do we need to retire the $GNOME_DESKTOP_SESSION_ID check here, thus penalizing people who might still be on an old version of GNOME? It doesn't seem like it would be a maintenance burden to continue checking it while also taking advantage of $XDG_CURRENT_DESKTOP:
if test -n "$GNOME_DESKTOP_SESSION_ID" || is_desktop GNOME
Ok, keeping this as well, it's even true that most people won't be in gnome 2 these days, but not a problem to keep it around.
Signature · 2 lines
Treviño's World - Life and Linux http://www.3v1n0.net
PATCH v2 1/2 mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME
In reply to Marco Trevisan via GitGitGadgetFrom: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <[email protected]>
To list merge tool candidates we used to use a private GNOME env variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and removed as part of GNOME 3.30.0 release [1].
So, git should instead primarily check the XDG_CURRENT_DESKTOP env variable, that is now supported by all the desktop environments.
Since the variable is actually a colon-separated list of names that the current desktop is known as, we need to check if the value is set if we're using GNOME.
[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65
--- git-mergetool--lib.sh | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
git-mergetool--lib.sh +12 -1
@@ -266,6 +266,17 @@ run_merge_cmd () { fi } +is_desktop () { + case ":$XDG_CURRENT_DESKTOP:" in + *:$1:*) + return 0 + ;; + *) + return 1 + ;; + esac +} + list_merge_tool_candidates () { if merge_mode then
@@ -275,7 +286,7 @@ list_merge_tool_candidates () { fi if test -n "$DISPLAY" then - if test -n "$GNOME_DESKTOP_SESSION_ID" + if is_desktop "GNOME" || test -n "$GNOME_DESKTOP_SESSION_ID" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools" else
Signature · 1 line
gitgitgadget
v2 cover mergetool-lib: Don't use deprecated variable to detect GNOME
In reply to Marco Trevisan via GitGitGadgetTo list merge tool candidates we used to use a private GNOME env variable ( GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and removed as part of GNOME 3.30.0 release [1].
So replace this using XDG_CURRENT_DESKTOP instead, and cleanup the code to avoid duplication and supporting KDE's kdiff3 better.
[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65
Marco Trevisan (Treviño) (2):
mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME
mergetool-lib: give kdiff3 priority in KDE environments
git-mergetool--lib.sh | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
base-commit: 85b4e0a6dc8407de6f69808d9ee6debdf167ced3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-693%2F3v1n0%2Fdesktop-envs-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-693/3v1n0/desktop-envs-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/693
Range-diff vs v1:
1: 66a026ae67 ! 1: 26b25b0d65 mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME
@@ Commit message
variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago
and removed as part of GNOME 3.30.0 release [1].
- So, git should instead check the XDG_CURRENT_DESKTOP env variable, that
- is supported by all the desktop environments.
+ So, git should instead primarily check the XDG_CURRENT_DESKTOP env variable,
+ that is now supported by all the desktop environments.
Since the variable is actually a colon-separated list of names that the current
- desktop is known as, we need to go through all the values to ensure
- we're using GNOME.
+ desktop is known as, we need to check if the value is set if we're using GNOME.
[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65
@@ git-mergetool--lib.sh: run_merge_cmd () {
}
+is_desktop () {
-+ IFS=':'
-+ for desktop in ${XDG_CURRENT_DESKTOP}
-+ do
-+ if test "$desktop" = "$1"
-+ then
-+ return 0
-+ fi
-+ done
-+
-+ return 1
++ case ":$XDG_CURRENT_DESKTOP:" in
++ *:$1:*)
++ return 0
++ ;;
++ *)
++ return 1
++ ;;
++ esac
+}
+
list_merge_tool_candidates () {
@@ git-mergetool--lib.sh: list_merge_tool_candidates () {
if test -n "$DISPLAY"
then
- if test -n "$GNOME_DESKTOP_SESSION_ID"
-+ if is_desktop "GNOME"
++ if is_desktop "GNOME" || test -n "$GNOME_DESKTOP_SESSION_ID"
then
tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
else
2: fc0d2b103e < -: ---------- mergetool-lib: keep a list of cross desktop merge tools
3: 37090d2322 ! 2: c18a5edf50 mergetool-lib: give kdiff3 prioirty in KDE environments @@ Metadata
Author: Marco Trevisan (Treviño) <[email protected]>
## Commit message ##
- mergetool-lib: give kdiff3 prioirty in KDE environments
+ mergetool-lib: give kdiff3 priority in KDE environments
## git-mergetool--lib.sh ##
@@ git-mergetool--lib.sh: list_merge_tool_candidates () {
- fi
- if test -n "$DISPLAY"
- then
-- cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff"
-+ cross_desktop_tools="opendiff tkdiff xxdiff"
- if is_desktop "GNOME"
+ if is_desktop "GNOME" || test -n "$GNOME_DESKTOP_SESSION_ID"
then
-- tools="meld $cross_desktop_tools $tools"
-+ tools="meld $cross_desktop_tools kdiff3 $tools"
-+ elif is_desktop "KDE"
+ tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
++ elif is_desktop "KDE" || test x"$KDE_FULL_SESSION" = x"true"
+ then
-+ tools="kdiff3 $cross_desktop_tools meld $tools"
++ tools="kdiff3 opendiff tkdiff xxdiff meld $tools"
else
-- tools="$cross_desktop_tools meld $tools"
-+ tools="$cross_desktop_tools kdiff3 meld $tools"
+ tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
- tools="$tools gvimdiff diffuse diffmerge ecmerge"
- tools="$tools p4merge araxis bc codecompare"
Signature · 1 line
gitgitgadget
PATCH v2 2/2 mergetool-lib: give kdiff3 priority in KDE environments
From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <[email protected]>
--- git-mergetool--lib.sh | 3 +++ 1 file changed, 3 insertions(+)
git-mergetool--lib.sh +3 -0
@@ -289,6 +289,9 @@ list_merge_tool_candidates () { if is_desktop "GNOME" || test -n "$GNOME_DESKTOP_SESSION_ID" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools" + elif is_desktop "KDE" || test x"$KDE_FULL_SESSION" = x"true" + then + tools="kdiff3 opendiff tkdiff xxdiff meld $tools" else tools="opendiff kdiff3 tkdiff xxdiff meld $tools" fi
Signature · 1 line
gitgitgadget
Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools
In reply to Eric SunshineIl giorno mer 5 ago 2020 alle ore 23:08 Eric Sunshine <[email protected]> ha scritto:
Quoted 19 lines
Quoted 16 lines · Marco Trevisan (Treviño) via GitGitGadget
Instead of repeating the same tools multiple times, let's just keep them in another variable and list them depending the current desktop
Signed-off-by Marco Trevisan (Treviño) <[email protected]>---
git-mergetool--lib.sh +2
-2@@ -288,11 +288,12 @@ list_merge_tool_candidates () { if test -n "$DISPLAY" then + cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff" if is_desktop "GNOME" then - tools="meld opendiff kdiff3 tkdiff xxdiff $tools" + tools="meld $cross_desktop_tools $tools" else - tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
Quoted 8 lines
git-mergetool--lib.sh list_merge_tool_candidates () { · line 297
Quoted 2 lines
git-mergetool--lib.sh +1
-0+ tools="$cross_desktop_tools meld $tools" fiI have mixed feelings about this change. On the one hand, I see the reason for doing it if the list of tools remains substantially the same in each case, but it also seems like it could become a burden, possibly requiring factoring the list into more pieces, as new platforms or tools are added.
I kind of agree on that, so it was mostly a proposal but I can withdraw it.
Quoted 5 lines
What I might find more compelling is creation of a table of tools and the platforms for which they are suitable. It doesn't seem like it would be too difficult to express such a table in shell and to extract the desired rows (but that might be overkill). At any rate, I'm rather "meh" on this change, though I don't oppose it strongly.
Yeah, I was thinking the same, but also could be a bit complicated to maintain especially when it comes using something that needs to be supported by pure sh.
Signature · 2 lines
Treviño's World - Life and Linux http://www.3v1n0.net
Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments
In reply to Eric SunshineIl giorno mer 5 ago 2020 alle ore 23:16 Eric Sunshine <[email protected]> ha scritto:
Quoted 20 lines
Quoted 1 line · Marco Trevisan (Treviño) via GitGitGadget
mergetool-lib: give kdiff3 prioirty in KDE environments
s/prioirty/priority/
Quoted 13 lines
Signed-off-by Marco Trevisan (Treviño) <[email protected]>---
git-mergetool--lib.sh +5
-2@@ -288,12 +288,15 @@ list_merge_tool_candidates () { - cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff" + cross_desktop_tools="opendiff tkdiff xxdiff" if is_desktop "GNOME" then - tools="meld $cross_desktop_tools $tools" + tools="meld $cross_desktop_tools kdiff3 $tools" + elif is_desktop "KDE" + then + tools="kdiff3 $cross_desktop_tools meld $tools"
Quoted 8 lines
git-mergetool--lib.sh list_merge_tool_candidates () { · line 300
Quoted 4 lines
git-mergetool--lib.sh +1
-1else - tools="$cross_desktop_tools meld $tools" + tools="$cross_desktop_tools kdiff3 meld $tools" fiWouldn't this change the behavior for people running old KDE which doesn't have
XDG_CURRENT_DESKTOP, giving "kdiff3" much lower priority than it had before?
Yeah, true.. So to avoid this we can just also check for KDE_FULL_SESSION, that has been introduced by KDE 3.2, and this should be enough I think.
Quoted 5 lines
This change also illustrates why I wasn't convinced that patch 2/3 was necessarily a good idea. When you touch '
cross_desktop_tools' here, you end up having to touch all the other 'tools=' lines anyhow, so the introduction of 'cross_desktop_tools' didn't buy much in terms of reduced maintenance cost.
Yeah, I had the same feeling, TBH.
Signature · 2 lines
Treviño's World - Life and Linux http://www.3v1n0.net
Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools
In reply to Johannes SixtHi,
Il giorno gio 6 ago 2020 alle ore 10:16 Johannes Sixt <[email protected]> ha scritto:
Quoted 7 lines
On a side note, the refactoring that happened in
21d0ba7ebb0c("difftool/mergetool: refactor commands to use git-mergetool--lib", 2009-04-08) did not preserve the original order of diff tools. The spirit was to prefer meld over kompare and kdiff3 with GNOME, and the other way round with KDE. But since that refactoring, kompare is always first in the list.
Well, actually when DISPLAY is set so basically always (as even under WAYLAND that's defined for XWayland apps) meld is still preferred.
I'm not sure it makes sense to have tortoisemerge or kompare if no display is set though, but I didn't want to change the whole thing.
Signature · 2 lines
Treviño's World - Life and Linux http://www.3v1n0.net