Display text if an image cannot be displayed #7868

gerhardol · 2020-03-15T22:34:01Z

See #7831 (comment)

Proposed changes

Display a text rather just an empty pane if the image has been removed or cannot be displayed for other reasons.

Screenshots

Before

After

Test methodology

Manual

✒️ I contribute this code under The Developer Certificate of Origin.

vbjay · 2020-03-16T01:23:12Z

Why can't we get the file from git and display that?

gerhardol · 2020-03-16T08:25:01Z

Why can't we get the file from git and display that?

This specific change is "low level", it could be that the file could not be displayed as an image (like a text file renamed to .png).

If there is a change, the logic to have special handling for removed images could be changed.
If so, I would like to do this in a follow up:

I would like to get RevisionDiff: Stage/Unstage selected lines #7825 and a followup that unifies the presentation of "changes" first
How should removed images be displayed, so the user understands that an image is removed?
For this reason, removed text files are displayed as a diff.
Generally, we spend too much time on discussing these edge situations, instead of just adding something that is good enough - what is that here?

vbjay · 2020-03-16T12:06:32Z

So the example was a removed image. My point is if that failed to show an image, why can't we show the original file? Yes. If we get an imageformat exception or whatnot then by all means show a text error message. If it is removed, we should write to temp the file and load that. If you are going to touch that logic then do it right. Why touch it twice.

gerhardol · 2020-03-16T13:08:20Z

Why touch it twice.

It is another part of the logic, and I have several PRs to clean up that first before making such a change.

mstv

👍

RussKie · 2020-03-19T00:31:43Z

GitUI/Editor/FileViewer.cs

+                                if (image == null)
+                                {
+                                    ResetForText(null);
+                                    internalFileViewer.SetText($"Cannot view image {fileName}", openWithDifftool);


Localise please

Done
Generally I believe translation is overkill for something like this, but I allow it as it is (at least currently) shown for removed images.
Unless there are further comments in the next two days, I will squash and schedule a merge

RussKie

A little change needed. Otherwise good

GitUI/Editor/FileViewer.cs

gerhardol · 2020-03-21T10:26:59Z

I updated the translation on top of #7877, still fails in master

GitUI/Translation/English.xlf

RussKie · 2020-03-21T21:07:46Z

I think it will fail if merged before...

mstv · 2020-03-21T21:28:31Z

I think it will fail if merged before...

So fix up the final nit and merge #7877, please, @RussKie. 😀

RussKie · 2020-03-21T21:43:00Z

It won't until late in the day. Feel free amend my PR, and squash-merge

GitUI/Editor/FileViewer.cs

gerhardol · 2020-03-22T11:32:10Z

@msftbot merge in 24 hours

ghost · 2020-03-22T11:32:14Z

Hello @gerhardol!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Mon, 23 Mar 2020 11:32:12 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

RussKie · 2020-03-22T13:27:32Z

Please do. There is no reason to depend on a 3rd party code here.

For instance removed images

ghost assigned gerhardol Mar 15, 2020

mstv approved these changes Mar 16, 2020

View reviewed changes

RussKie requested changes Mar 19, 2020

View reviewed changes

ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 19, 2020

gerhardol force-pushed the bugfix/removed-image branch from 0ba8a7e to d6b8af1 Compare March 20, 2020 23:07

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 20, 2020

RussKie reviewed Mar 21, 2020

View reviewed changes

GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved

gerhardol force-pushed the bugfix/removed-image branch from d6b8af1 to 3369c0b Compare March 21, 2020 10:16

mstv reviewed Mar 21, 2020

View reviewed changes

GitUI/Translation/English.xlf Outdated Show resolved Hide resolved

gerhardol force-pushed the bugfix/removed-image branch from 3369c0b to 192b992 Compare March 22, 2020 10:03

RussKie reviewed Mar 22, 2020

View reviewed changes

GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved

RussKie approved these changes Mar 22, 2020

View reviewed changes

ghost added the status: auto merge label Mar 22, 2020

Display text if an image cannot be displayed

1705600

For instance removed images

gerhardol force-pushed the bugfix/removed-image branch from 192b992 to 1705600 Compare March 22, 2020 13:37

gerhardol mentioned this pull request Mar 22, 2020

RevisionDiff: Stage/Unstage selected lines #7825

Merged

RussKie approved these changes Mar 23, 2020

View reviewed changes

ghost merged commit 39bb7a2 into gitextensions:master Mar 23, 2020

ghost added this to the 4.0 milestone Mar 23, 2020

gerhardol deleted the bugfix/removed-image branch November 14, 2021 13:01

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display text if an image cannot be displayed #7868

Display text if an image cannot be displayed #7868

gerhardol commented Mar 15, 2020

vbjay commented Mar 16, 2020 via email •

edited by ghost

Loading

gerhardol commented Mar 16, 2020

vbjay commented Mar 16, 2020 via email •

edited by ghost

Loading

gerhardol commented Mar 16, 2020

mstv left a comment

RussKie Mar 19, 2020

gerhardol Mar 20, 2020

RussKie left a comment

gerhardol commented Mar 21, 2020

RussKie commented Mar 21, 2020 via email •

edited by ghost

Loading

mstv commented Mar 21, 2020

RussKie commented Mar 21, 2020 via email •

edited by ghost

Loading

gerhardol commented Mar 22, 2020

ghost commented Mar 22, 2020

RussKie commented Mar 22, 2020 via email •

edited by ghost

Loading

Display text if an image cannot be displayed #7868

Display text if an image cannot be displayed #7868

Conversation

gerhardol commented Mar 15, 2020

Proposed changes

Screenshots

Before

After

Test methodology

vbjay commented Mar 16, 2020 via email • edited by ghost Loading

gerhardol commented Mar 16, 2020

vbjay commented Mar 16, 2020 via email • edited by ghost Loading

gerhardol commented Mar 16, 2020

mstv left a comment

Choose a reason for hiding this comment

RussKie Mar 19, 2020

Choose a reason for hiding this comment

gerhardol Mar 20, 2020

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

gerhardol commented Mar 21, 2020

RussKie commented Mar 21, 2020 via email • edited by ghost Loading

mstv commented Mar 21, 2020

RussKie commented Mar 21, 2020 via email • edited by ghost Loading

gerhardol commented Mar 22, 2020

ghost commented Mar 22, 2020

RussKie commented Mar 22, 2020 via email • edited by ghost Loading

vbjay commented Mar 16, 2020 via email •

edited by ghost

Loading

vbjay commented Mar 16, 2020 via email •

edited by ghost

Loading

RussKie commented Mar 21, 2020 via email •

edited by ghost

Loading

RussKie commented Mar 21, 2020 via email •

edited by ghost

Loading

RussKie commented Mar 22, 2020 via email •

edited by ghost

Loading