Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full Quick Look Support. #1815

Closed
wants to merge 6 commits into from

Conversation

LeonardoLarranaga
Copy link
Contributor

Description

This pull request brings full Quick Look support. This means that now, any type of file compatible with Quick Look can be viewed within CodeEdit (such as videos).

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

Videos inside CodeEdit

Menu

Quick Look Menu

Video

Screen.Recording.2024-07-10.at.23.04.25.mov

Update 1

Opened in code editor (left). Opened with Quick Look (right).
Icon when opening with Quick Look.

Update 2

Open As -> Source Code menu item is now enabled. When having a file opened with Quick Look, clicking this option will opened the file in the default CodeEdit editor.

Screen.Recording.2024-07-10.at.23.50.55.mov

@austincondiff
Copy link
Collaborator

austincondiff commented Jul 28, 2024

Great work! A few small things...

  1. I am not sure if opening QuickLook externally is necessary, it just adds another level to the menu which means more time to get to the end item selection.
  2. I think the eye icon on the tab is a bit much because it competes with the main icon.

@LeonardoLarranaga
Copy link
Contributor Author

@austincondiff

  1. I think the eye icon on the tab is a bit much because it competes with the main icon.

What about an option in the Settings window? So we can let the user decide.

@austincondiff
Copy link
Collaborator

I'd rather keep settings as concise as possible. Xcode doesn't include this, I'd like to follow suit if you wouldn't mind.

austincondiff
austincondiff previously approved these changes Aug 8, 2024
Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Member

@tom-ludwig tom-ludwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like once I’ve opened the file in Quick Look, I’m unable to switch back to viewing it as source code.
CE:
Screenshot 2024-08-14 at 2 21 47 PM
Xcode:
Screenshot 2024-08-14 at 2 22 01 PM

@LeonardoLarranaga
Copy link
Contributor Author

LeonardoLarranaga commented Aug 14, 2024

@activcoding This seems to be an issue with that way the conditional for showing Source Code works. if type.conforms(to: .sourceCode) doesn't appear to work for all text files, such as .md.

Should I go ahead and change it to if type.conforms(to: .text)? Or should the Source Code option always appear?

Running in a Playground the the following code for .sourceCode, we are able to see which extensions belong to that UTType.

  1. s
  2. c
  3. m
  4. swift
  5. cp
  6. cpp
  7. c++
  8. cc
  9. cxx
  10. mm
  11. h
  12. hh
  13. hp
  14. hpp
  15. hxx
  16. h++
  17. ipp
  18. applescript
  19. scpt
  20. js
  21. jscript
  22. javascript
  23. mjs
  24. sh
  25. pl
  26. pm
  27. py
  28. rb
  29. rbw
  30. php
  31. php3
  32. php4
  33. ph3
  34. ph4
  35. phtml
  36. pl
  37. pm
  38. py
  39. rb
  40. rbw
  41. php
  42. php3
  43. php4
  44. ph3
  45. ph4
  46. phtml
  47. make
  48. mak
  49. gmk
  50. mk
  51. applescript
  52. scpt
  53. js
  54. jscript
  55. javascript
  56. mjs
  57. sh
  58. pl
  59. pm
  60. py
  61. rb
  62. rbw
  63. php
  64. php3
  65. php4
  66. ph3
  67. ph4
  68. phtml
  69. pl
  70. pm
  71. py
  72. rb
  73. rbw
  74. php
  75. php3
  76. php4
  77. ph3
  78. ph4
  79. phtml
  80. make
  81. mak
  82. gmk
  83. mk

@FastestMolasses
Copy link
Member

Do we need the Quick Look dropdown? It seems like an unnecessary step. Since it's only 2 options, we can just display those two as "Quick Look in Modal" or "Quick Look in Tab". Thoughts?

@LeonardoLarranaga
Copy link
Contributor Author

@FastestMolasses, Open In Modal ways removed as suggested by @austincondiff. See Removed Quick Look icon, removed Open In Modal option.

My previous comment highlights a current issue, and I’m waiting for a response on that so I can move forward with the edits.

@austincondiff
Copy link
Collaborator

Should I go ahead and change it to if type.conforms(to: .text)? Or should the Source Code option always appear?

My personal opinion is this is a text editor so editing text should be prioritized and should be default. I don't think we should allow only a specified set of file types to edit text. There will always be new and/or custom filetypes that we are not aware of that needs to be editable via text.

That said I think this option should be available where possible. Now I can see that potentially being problematic when dealing with images and videos where it is not possible to edit them in this way and if this is the case, disabling this option might be a good thing.

@CodeEditApp/maintainers thoughts on this?

@LeonardoLarranaga
Copy link
Contributor Author

Sorry about that. Had some conflicts with my fork but they're now fixed.

@0xWDG
Copy link
Collaborator

0xWDG commented Sep 18, 2024

That said I think this option should be available where possible. Now I can see that potentially being problematic when dealing with images and videos where it is not possible to edit them in this way and if this is the case, disabling this option might be a good thing.

Text editting should be the priority.

@LeonardoLarranaga
Copy link
Contributor Author

Closing this for now as it no longer works with the current CodeFileDocument implementation. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Quick Look support
5 participants