* [PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME @ 2020-08-05 13:34 Marco Trevisan via GitGitGadget 2020-08-05 13:34 ` [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Marco Trevisan via GitGitGadget @ 2020-08-05 13:34 UTC (permalink / raw) To: git; +Cc: Marco Trevisan 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 -- gitgitgadget ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME 2020-08-05 13:34 [PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget @ 2020-08-05 13:34 ` Marco Trevisan (Treviño) via GitGitGadget 2020-08-05 20:58 ` Eric Sunshine 2020-08-05 13:34 ` [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools Marco Trevisan (Treviño) via GitGitGadget ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-05 13:34 UTC (permalink / raw) To: git; +Cc: Marco Trevisan, Marco Trevisan (Treviño) From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <mail@3v1n0.net> 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 Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> --- git-mergetool--lib.sh | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 204a5acd66..be28fe375f 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -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 -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME 2020-08-05 13:34 ` [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-05 20:58 ` Eric Sunshine 2020-08-05 22:14 ` Junio C Hamano 2020-08-06 12:12 ` Marco Trevisan (Treviño) 0 siblings, 2 replies; 17+ messages in thread From: Eric Sunshine @ 2020-08-05 20:58 UTC (permalink / raw) To: Marco Trevisan (Treviño) via GitGitGadget Cc: Git List, Marco Trevisan (Treviño) On Wed, Aug 5, 2020 at 3:51 PM Marco Trevisan (Treviño) via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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. > > Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> > --- > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > @@ -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 > + 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? > @@ -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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME 2020-08-05 20:58 ` Eric Sunshine @ 2020-08-05 22:14 ` Junio C Hamano 2020-08-06 12:12 ` Marco Trevisan (Treviño) 1 sibling, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2020-08-05 22:14 UTC (permalink / raw) To: Eric Sunshine Cc: Marco Trevisan (Treviño) via GitGitGadget, Git List, Marco Trevisan (Treviño) Eric Sunshine <sunshine@sunshineco.com> writes: > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME 2020-08-05 20:58 ` Eric Sunshine 2020-08-05 22:14 ` Junio C Hamano @ 2020-08-06 12:12 ` Marco Trevisan (Treviño) 1 sibling, 0 replies; 17+ messages in thread From: Marco Trevisan (Treviño) @ 2020-08-06 12:12 UTC (permalink / raw) To: Eric Sunshine; +Cc: Marco Trevisan (Treviño) via GitGitGadget, Git List Hi, Il giorno mer 5 ago 2020 alle ore 22:58 Eric Sunshine <sunshine@sunshineco.com> ha scritto: > > On Wed, Aug 5, 2020 at 3:51 PM Marco Trevisan (Treviño) via > GitGitGadget <gitgitgadget@gmail.com> wrote: > > 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. > > > > Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> > > --- > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > > @@ -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 Yeah, that's true and always a good practice, but it was never happening in this file, so I followed the style. > > + 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? Ok, fair enough, this is fine as well. > > @@ -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. -- Treviño's World - Life and Linux http://www.3v1n0.net ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools 2020-08-05 13:34 [PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget 2020-08-05 13:34 ` [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-05 13:34 ` Marco Trevisan (Treviño) via GitGitGadget 2020-08-05 21:08 ` Eric Sunshine 2020-08-05 13:34 ` [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments Marco Trevisan (Treviño) via GitGitGadget 2020-08-06 15:27 ` [PATCH v2 0/2] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget 3 siblings, 1 reply; 17+ messages in thread From: Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-05 13:34 UTC (permalink / raw) To: git; +Cc: Marco Trevisan, Marco Trevisan (Treviño) From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <mail@3v1n0.net> 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) <mail@3v1n0.net> --- git-mergetool--lib.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index be28fe375f..243cd2b06b 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -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" -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools 2020-08-05 13:34 ` [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-05 21:08 ` Eric Sunshine 2020-08-06 8:16 ` Johannes Sixt 2020-08-06 12:28 ` Marco Trevisan (Treviño) 0 siblings, 2 replies; 17+ messages in thread From: Eric Sunshine @ 2020-08-05 21:08 UTC (permalink / raw) To: Marco Trevisan (Treviño) via GitGitGadget Cc: Git List, Marco Trevisan (Treviño) On Wed, Aug 5, 2020 at 3:53 PM Marco Trevisan (Treviño) via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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) <mail@3v1n0.net> > --- > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > @@ -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" > + 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools 2020-08-05 21:08 ` Eric Sunshine @ 2020-08-06 8:16 ` Johannes Sixt 2020-08-06 12:19 ` Marco Trevisan (Treviño) 2020-08-06 12:28 ` Marco Trevisan (Treviño) 1 sibling, 1 reply; 17+ messages in thread From: Johannes Sixt @ 2020-08-06 8:16 UTC (permalink / raw) To: Eric Sunshine, Marco Trevisan (Treviño) via GitGitGadget Cc: Git List, Marco Trevisan (Treviño) Am 05.08.20 um 23:08 schrieb Eric Sunshine: > On Wed, Aug 5, 2020 at 3:53 PM Marco Trevisan (Treviño) via > GitGitGadget <gitgitgadget@gmail.com> wrote: >> 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) <mail@3v1n0.net> >> --- >> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh >> @@ -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" >> + 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. 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools 2020-08-06 8:16 ` Johannes Sixt @ 2020-08-06 12:19 ` Marco Trevisan (Treviño) 0 siblings, 0 replies; 17+ messages in thread From: Marco Trevisan (Treviño) @ 2020-08-06 12:19 UTC (permalink / raw) To: Johannes Sixt Cc: Eric Sunshine, Marco Trevisan (Treviño) via GitGitGadget, Git List Hi, Il giorno gio 6 ago 2020 alle ore 10:16 Johannes Sixt <j6t@kdbg.org> ha scritto: > > 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. -- Treviño's World - Life and Linux http://www.3v1n0.net ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools 2020-08-05 21:08 ` Eric Sunshine 2020-08-06 8:16 ` Johannes Sixt @ 2020-08-06 12:28 ` Marco Trevisan (Treviño) 1 sibling, 0 replies; 17+ messages in thread From: Marco Trevisan (Treviño) @ 2020-08-06 12:28 UTC (permalink / raw) To: Eric Sunshine; +Cc: Marco Trevisan (Treviño) via GitGitGadget, Git List Il giorno mer 5 ago 2020 alle ore 23:08 Eric Sunshine <sunshine@sunshineco.com> ha scritto: > > On Wed, Aug 5, 2020 at 3:53 PM Marco Trevisan (Treviño) via > GitGitGadget <gitgitgadget@gmail.com> wrote: > > 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) <mail@3v1n0.net> > > --- > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > > @@ -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" > > + 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. I kind of agree on that, so it was mostly a proposal but I can withdraw it. > 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. -- Treviño's World - Life and Linux http://www.3v1n0.net ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments 2020-08-05 13:34 [PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget 2020-08-05 13:34 ` [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget 2020-08-05 13:34 ` [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-05 13:34 ` Marco Trevisan (Treviño) via GitGitGadget 2020-08-05 21:15 ` Eric Sunshine 2020-08-06 15:27 ` [PATCH v2 0/2] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget 3 siblings, 1 reply; 17+ messages in thread From: Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-05 13:34 UTC (permalink / raw) To: git; +Cc: Marco Trevisan, Marco Trevisan (Treviño) From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <mail@3v1n0.net> Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> --- git-mergetool--lib.sh | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 243cd2b06b..90de6ee884 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -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" -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments 2020-08-05 13:34 ` [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-05 21:15 ` Eric Sunshine 2020-08-06 12:36 ` Marco Trevisan (Treviño) 0 siblings, 1 reply; 17+ messages in thread From: Eric Sunshine @ 2020-08-05 21:15 UTC (permalink / raw) To: Marco Trevisan (Treviño) via GitGitGadget Cc: Git List, Marco Trevisan (Treviño) On Wed, Aug 5, 2020 at 4:02 PM Marco Trevisan (Treviño) via GitGitGadget <gitgitgadget@gmail.com> wrote: > mergetool-lib: give kdiff3 prioirty in KDE environments s/prioirty/priority/ > Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> > --- > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > @@ -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" > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments 2020-08-05 21:15 ` Eric Sunshine @ 2020-08-06 12:36 ` Marco Trevisan (Treviño) 2020-08-06 16:17 ` Eric Sunshine 0 siblings, 1 reply; 17+ messages in thread From: Marco Trevisan (Treviño) @ 2020-08-06 12:36 UTC (permalink / raw) To: Eric Sunshine; +Cc: Marco Trevisan (Treviño) via GitGitGadget, Git List Il giorno mer 5 ago 2020 alle ore 23:16 Eric Sunshine <sunshine@sunshineco.com> ha scritto: > > On Wed, Aug 5, 2020 at 4:02 PM Marco Trevisan (Treviño) via > GitGitGadget <gitgitgadget@gmail.com> wrote: > > mergetool-lib: give kdiff3 prioirty in KDE environments > > s/prioirty/priority/ > > > Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> > > --- > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > > @@ -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" > > 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? 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. > 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. -- Treviño's World - Life and Linux http://www.3v1n0.net ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments 2020-08-06 12:36 ` Marco Trevisan (Treviño) @ 2020-08-06 16:17 ` Eric Sunshine 0 siblings, 0 replies; 17+ messages in thread From: Eric Sunshine @ 2020-08-06 16:17 UTC (permalink / raw) To: Marco Trevisan (Treviño) Cc: Marco Trevisan (Treviño) via GitGitGadget, Git List On Thu, Aug 6, 2020 at 8:37 AM Marco Trevisan (Treviño) <mail@3v1n0.net> wrote: > Il giorno mer 5 ago 2020 alle ore 23:16 Eric Sunshine > <sunshine@sunshineco.com> ha scritto: > > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/2] mergetool-lib: Don't use deprecated variable to detect GNOME 2020-08-05 13:34 [PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget ` (2 preceding siblings ...) 2020-08-05 13:34 ` [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-06 15:27 ` Marco Trevisan via GitGitGadget 2020-08-06 15:27 ` [PATCH v2 1/2] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget 2020-08-06 15:27 ` [PATCH v2 2/2] mergetool-lib: give kdiff3 priority in KDE environments Marco Trevisan (Treviño) via GitGitGadget 3 siblings, 2 replies; 17+ messages in thread From: Marco Trevisan via GitGitGadget @ 2020-08-06 15:27 UTC (permalink / raw) To: git; +Cc: Marco Trevisan 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) (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) <mail@3v1n0.net> ## Commit message ## - mergetool-lib: give kdiff3 prioirty in KDE environments + mergetool-lib: give kdiff3 priority in KDE environments Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> ## 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" -- gitgitgadget ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME 2020-08-06 15:27 ` [PATCH v2 0/2] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget @ 2020-08-06 15:27 ` Marco Trevisan (Treviño) via GitGitGadget 2020-08-06 15:27 ` [PATCH v2 2/2] mergetool-lib: give kdiff3 priority in KDE environments Marco Trevisan (Treviño) via GitGitGadget 1 sibling, 0 replies; 17+ messages in thread From: Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-06 15:27 UTC (permalink / raw) To: git; +Cc: Marco Trevisan, Marco Trevisan (Treviño) From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <mail@3v1n0.net> 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 Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> --- git-mergetool--lib.sh | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 204a5acd66..f9d8f309c8 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -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 -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] mergetool-lib: give kdiff3 priority in KDE environments 2020-08-06 15:27 ` [PATCH v2 0/2] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget 2020-08-06 15:27 ` [PATCH v2 1/2] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-06 15:27 ` Marco Trevisan (Treviño) via GitGitGadget 1 sibling, 0 replies; 17+ messages in thread From: Marco Trevisan (Treviño) via GitGitGadget @ 2020-08-06 15:27 UTC (permalink / raw) To: git; +Cc: Marco Trevisan, Marco Trevisan (Treviño) From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <mail@3v1n0.net> Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net> --- git-mergetool--lib.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index f9d8f309c8..ac6695ea26 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -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 -- gitgitgadget ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-08-06 17:29 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-05 13:34 [PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget 2020-08-05 13:34 ` [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget 2020-08-05 20:58 ` Eric Sunshine 2020-08-05 22:14 ` Junio C Hamano 2020-08-06 12:12 ` Marco Trevisan (Treviño) 2020-08-05 13:34 ` [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools Marco Trevisan (Treviño) via GitGitGadget 2020-08-05 21:08 ` Eric Sunshine 2020-08-06 8:16 ` Johannes Sixt 2020-08-06 12:19 ` Marco Trevisan (Treviño) 2020-08-06 12:28 ` Marco Trevisan (Treviño) 2020-08-05 13:34 ` [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments Marco Trevisan (Treviño) via GitGitGadget 2020-08-05 21:15 ` Eric Sunshine 2020-08-06 12:36 ` Marco Trevisan (Treviño) 2020-08-06 16:17 ` Eric Sunshine 2020-08-06 15:27 ` [PATCH v2 0/2] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget 2020-08-06 15:27 ` [PATCH v2 1/2] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget 2020-08-06 15:27 ` [PATCH v2 2/2] mergetool-lib: give kdiff3 priority in KDE environments Marco Trevisan (Treviño) via GitGitGadget
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.