| Message ID | fc0d2b103ec080fa38e5d51bf2205b7360c1b601.1596634463.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mergetool-lib: Don't use deprecated variable to detect GNOME | expand |
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.
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
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.
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.
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"