Compare snapshots: Don't use "false" as fallback diff command

[buhtz]

backintime/qt/snapshotsdialog.py

Lines 34 to 42 in ed282ad

	if tools.checkCommand('meld'):
	DIFF_CMD = 'meld'
	DIFF_PARAMS = '%1 %2'
	elif tools.checkCommand('kompare'):
	DIFF_CMD = 'kompare'
	DIFF_PARAMS = '%1 %2'
	else:
	DIFF_CMD = 'false'
	DIFF_PARAMS = '%1 %2'

Based on report on bit-dev mailing list.

Problem

An extern application is used to compare snapshots. If meld and kompare as such tools are not installed BIT is falling back to false. Be aware that false is not False and that false is a valid command in a *nix shell (see $ manpage false). This makes no sense from users perspective. The "Compare" button just does nothing in this case. No reaction. No response to the user.

BIT doesn't check for false but just checks for availability of the configured diff command.

backintime/qt/snapshotsdialog.py

Lines 336 to 340 in ed282ad

	if not tools.checkCommand(diffCmd):
	messagebox.critical(
	self, '{}: {}'.format(_('Command not found'), diffCmd)
	)
	return

Solution

The diff command should be empty (or None) if meld or kompare are not available. And the Compare button should check for emptiness.
Also the error message could be improved. A simple "Command not found" is not user friendly and to technical.

[stcksmsh]

if tools.checkCommand('meld'):
    DIFF_CMD = 'meld'
    DIFF_PARAMS = '%1 %2'
elif tools.checkCommand('kompare'):
    DIFF_CMD = 'kompare'
    DIFF_PARAMS = '%1 %2'
else:
    DIFF_CMD = ''
    DIFF_PARAMS = '%1 %2'

        if not diffCmd:
            messagebox.critical(
                self, ('No diff command found. Please install meld or kompare.'))
            return

This is my idea of the fix, but I have a question (which probably does not make too much sense):
Based on the mailing list report we can see users want to be able to use non gui compare tools, an example of diff is given in the email. Right now if someone were to set the command to diff and the parameters to -r %1 %2 which should be a valid diff command, it will simply do nothing. Maybe just show a messagebox.info with the output (if there is anything to show, as kompare and meld do not seem to 'give' anything to stdout)?

[buhtz]

Hello Kosta,
thanks for jumping in here.

Your approach seems correct to me. I would suggest a slightly refactoring like this:

if tools.checkCommand('meld'):
    DIFF_CMD = 'meld'
elif tools.checkCommand('kompare'):
    DIFF_CMD = 'kompare'
else:
    DIFF_CMD = None

if DIFF_CMD is None:
    DIFF_PARAMS = None
else:
    DIFF_PARAMS = '%1 %2'

And the dialog:

    if diffCmd is None:
        messagebox.critical(
            self, ('No tool found to compare snapshots. It is suggested to install "meld", "kompare", or a similar tool.'))
        return

Based on the mailing list report we can see users want to be able to use non gui compare tools
[..]
if someone were to set the command to diff and the parameters to -r %1 %2 which should be a valid diff command, it will simply do nothing.

I would treat this as an edge case. The users are responsible for this them self. When they want a terminal based diff output they have to use the correct commands (some bash/sh, termemu magic). Just ignore this case.

Maybe just show a messagebox.info with the output (if there is anything to show, as kompare and meld do not seem to 'give' anything to stdout)?

We could check the output on stdout when using subprocess.run(). But this is to much code and complexity for an edge case (1 user in 100.000 maybe). My intention is to simplify the the old BIT code where ever it is possible to improve its maintainablity. 😄