Fix open unsupported Documents, support for open image files by shibotong · Pull Request #679 · CodeEditApp/CodeEdit

shibotong · 2022-06-29T11:54:25Z

Description

What's been implemented:

*make unsupported filed can be opened and show error message
*add support for opening image files (#674)

Related Issue

✨ Open and Preview Image, Audio, and Video as Tab #674

Checklist

I read and understood the contributing guide as well as the code of conduct
UI support for Binary Files
support for images
My changes generate no new warnings
My code builds and runs on my machine
I documented my code
Review requested

Screenshots

unsupported document should be opened and show unsupport

austincondiff · 2022-07-05T15:53:35Z

Nice work! What happens when the image is larger than the viewport? Does it scale to fit or will it scroll? We may at some point consider zoom controls.

austincondiff · 2022-07-05T16:16:19Z

We also shouldn't use the same text found in VS Code...

I tested howe Xcode handles this and threw many different filetypes at it. It attempts to render just about anything as it does in Quick Look and this also includes images. Sometimes it only shows the file icon in the case of files such as zip. I would try to replicate this if possible.

You may take a look at QLPreviewController.

0xWDG

LGTM

shibotong · 2022-07-05T21:36:29Z

If the image is larger than view port, it will become a scroll view with vertical direction and its width will fit for view port @austincondiff

Thank you for your suggestion of that package, I'll keep working on this,

austincondiff · 2022-07-05T23:09:27Z

I just update my comment above. Take a look at QuickLookUI instead as this is for macOS.

austincondiff · 2022-07-05T23:14:55Z

You may also look at QLPreviewView as well because you don't need the QuickLook window (I still don't know much about this, but this looks about right).

shibotong · 2022-07-10T08:11:01Z

Hi @austincondiff
I have updated the code. Now all type of files (except image and text) are opened with QLPreviewView

austincondiff · 2022-07-10T13:38:07Z

Very nice! Why not open images with QLPreviewView too?

shibotong · 2022-07-10T23:25:33Z

Good idea, I haven’t think about that. If do that way, we only have 2 different type of file. One is text file and the other is “other type”. Regards, Shibo Tong

…

On 10 Jul 2022, at 23:38, Austin Condiff ***@***.***> wrote: Why not open images with QLPreviewView too? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

shibotong · 2022-07-12T23:39:41Z

Hi @austincondiff
If open images with QLPreviewView, the image with lower resolution could be like this

austincondiff · 2022-07-13T03:12:09Z

You might be able to shrink and center the view to inherit the dimension of the images pixel dimension. You might do a check and see if the image width is less than the viewport width set the views maximum width to the image width, then do the same with the height. This way the image can only scale up to its natural pixel dimension but if the viewport is resized smaller, it can scale down. Does that make sense?

Matejkob

Great work with this feature! I have a few questions and some suggestions for your code. I'd love to hear your feedback to my comments! 💙

Matejkob · 2022-07-13T07:34:06Z

+        CodeFileView(codeFile: codeFile)
+            .safeAreaInset(edge: .top, spacing: 0) {
+                VStack(spacing: 0) {
+                    BreadcrumbsView(file: item, tappedOpenFile: workspace.openTab(item:))
+                    Divider()
+                }
+            }


I noticed that in the other two methods, instead of .safeAreaInset(edge:), you used only VStack. I believe this also should work correctly here. @wdg @pkasila @lilingxi01 @lukepistrol @StefKors @austincondiff, does anyone know why someone decided to use .safeAreaInset(edge:) instead of code like this:

VStack(spacing: 0) { BreadcrumbsView(file: item, tappedOpenFile: workspace.openTab(item:)) Divider() CodeFileView(codeFile: codeFile) }

I'm wondering if we can make those three views more consistent.

This is because if I use safeAreaInset() here, images in ScrollView will have a 1 pixel space on the top.

I think using VStack is making more sense.

I fully agree with you, and I think we should use only VStack. Using safeAreaInset seems to be unneeded to me.

@shibotong @Matejkob We need to use safeAreaInset so that we will see the blurred vibrancy effect in the toolbar when there is overflow. The above screenshot reveals a defect in the tab bar that needs its own issue and needs to be fixed.

Thank you @austincondiff I have changed these views to safeAreaInset. I think its ready to merge now

Matejkob · 2022-07-13T10:49:59Z

    @Published
    var content = ""

+    @Published
+    var image: NSImage?


I wonder about these two properties. Am I correct that both of them should not be set simultaneously? I can think only about the scenario where one is set, and the other is not. If my thinking is correct, we can consider merging them into one property. It can be achieved using enum, something like this:

enum ContentType { case text(String) case image(NSImage) } @Published var content: Content? // or var content: Content = .text("")

Thanks to that solution API of this class is more consistent, and we are assured that an undefined state where image and content are set simultaneously.

Good Idea. Thank you so much for this comment

Hi @Matejkob
I like this idea, but I am having issue with unwrap content to image or text. Could you give me an example of unwrapping context to Binding ?

Hi @shibotong,
may you try something like this:

enum Content { case string(String) case int(Int) } class Document: ObservableObject { @Published var content: Content? = nil } let document = Document() func string(value: Binding<String>) {} if let content = document.content { switch content { case let .string(value): string( value: Binding( get: { value }, set: { document.content = .string($0) } ) ) case let .int(value): break } }

Thank you for your reply. I think we need to use QLPreviewView to open images and don't need to use enum for content. Thank you for your help

Matejkob · 2022-07-13T10:53:58Z

@@ -44,8 +84,24 @@ public final class CodeFileDocument: NSDocument, ObservableObject {
        return data
    }


If image is set instead of content, should we not return data of the image here?

jiengup · 2022-07-13T11:41:16Z

Glad to receive your letter.I will respond ASAP.Thanks again!

austincondiff · 2022-07-16T16:23:54Z

@shibotong Does all of this make sense? Are you able to resolve these issues so we can get this merged in?

shibotong · 2022-07-17T04:02:46Z

You might be able to shrink and center the view to inherit the dimension of the images pixel dimension. You might do a check and see if the image width is less than the viewport width set the views maximum width to the image width, then do the same with the height. This way the image can only scale up to its natural pixel dimension but if the viewport is resized smaller, it can scale down. Does that make sense?

That's make sense, but that's the reason of can't use QLPreviewView for images. QLPreviewView only take the URL of file.
If we would like to detect image pixel, I think treat image as image is better to treat image as other type of file

Co-authored-by: Mateusz Bąk <44930823+Matejkob@users.noreply.github.com>

shibotong · 2022-07-18T00:49:54Z

Hi @austincondiff its ready to merge now. Thanks

austincondiff · 2022-07-18T05:59:53Z

@allcontributors add @shibotong for code

allcontributors · 2022-07-18T06:00:01Z

@austincondiff

I've put up a pull request to add @shibotong! 🎉

shibotong added 9 commits June 29, 2022 21:37

Add unsupported file view support

5f07476

open unsupport document

1e1a4cd

unsupported document should be opened and show unsupport

fix line length

61b8ec8

Oops, remove empty line

2d11854

support image documents

650b44b

Delete empty line

64e9610

Remove TODO

2e08efa

Add Documentation

1e5ecb3

Fix line limit

f5ac190

shibotong marked this pull request as ready for review June 30, 2022 02:08

shibotong changed the title ~~[WIP]Fix open unsupported Documents, support for open image files~~ Fix open unsupported Documents, support for open image files Jun 30, 2022

austincondiff requested review from 0xWDG, StefKors, lilingxi01, lukepistrol and pkasila July 5, 2022 15:51

0xWDG previously approved these changes Jul 5, 2022

View reviewed changes

pkasila previously approved these changes Jul 5, 2022

View reviewed changes

shibotong added 2 commits July 10, 2022 11:47

Merge remote-tracking branch 'upstream/main' into Image-View-Support

6e6e4f9

Use QLPreviewView to support unsupported files

88aa2e1

shibotong dismissed stale reviews from pkasila and 0xWDG via 88aa2e1 July 10, 2022 08:04

shibotong added 2 commits July 10, 2022 18:05

update preview item in OtherFileView

32f25d9

Update CodeFile.swift

0246c85

Matejkob reviewed Jul 13, 2022

View reviewed changes

shibotong and others added 3 commits July 17, 2022 14:12

Update CodeEditModules/Modules/CodeFile/src/CodeFile.swift

a79c57b

Co-authored-by: Mateusz Bąk <44930823+Matejkob@users.noreply.github.com>

Update CodeEditModules/Modules/CodeFile/src/CodeFile.swift

228d4e4

Co-authored-by: Mateusz Bąk <44930823+Matejkob@users.noreply.github.com>

Update open with QLPreviewView

f46ea19

shibotong added 2 commits July 18, 2022 14:34

Use SafeAreaInset for codefile and other file view

3c46eef

Fix CircleCI Line Issue

04f65c0

austincondiff merged commit b6dd9bb into CodeEditApp:main Jul 18, 2022

allcontributors Bot mentioned this pull request Jul 18, 2022

docs: add shibotong as a contributor for code #688

Merged

0xWDG mentioned this pull request Jul 26, 2022

✨ Open and Preview Image, Audio, and Video as Tab #674

Closed

		@@ -44,8 +84,24 @@ public final class CodeFileDocument: NSDocument, ObservableObject {
		return data
		}

Conversation

shibotong commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Checklist

Screenshots

Uh oh!

austincondiff commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

austincondiff commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xWDG left a comment

Choose a reason for hiding this comment

Uh oh!

shibotong commented Jul 5, 2022

Uh oh!

austincondiff commented Jul 5, 2022

Uh oh!

austincondiff commented Jul 5, 2022

Uh oh!

shibotong commented Jul 10, 2022

Uh oh!

austincondiff commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shibotong commented Jul 10, 2022 via email

Uh oh!

shibotong commented Jul 12, 2022

Uh oh!

austincondiff commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matejkob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austincondiff Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jiengup commented Jul 13, 2022 via email

Uh oh!

austincondiff commented Jul 16, 2022

Uh oh!

shibotong commented Jul 17, 2022

Uh oh!

shibotong commented Jul 18, 2022

Uh oh!

austincondiff commented Jul 18, 2022

Uh oh!

allcontributors Bot commented Jul 18, 2022

Uh oh!

Reviewers

Uh oh!

shibotong commented Jun 29, 2022 •

edited

Loading

austincondiff commented Jul 5, 2022 •

edited

Loading

austincondiff commented Jul 5, 2022 •

edited

Loading

austincondiff commented Jul 10, 2022 •

edited

Loading

austincondiff commented Jul 13, 2022 •

edited

Loading

austincondiff Jul 18, 2022 •

edited

Loading