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

WIP: Links to non-org files go to GitLab #876

Closed
wants to merge 1 commit into from

Conversation

kindrowboat
Copy link

This work in progress / proof-of-concept shows a potential solution for
links to files that are not org files or folders (#325). It does this by:

  • saving the URL to the GitLab project at login time
  • parsing links to files that contain a "." but are not org files
  • making a link to the file with the template
    ${gitLablProjectUrl}/-/blob/${defaultBranch}/${path}

The commit is not production ready and has not added or updated any of
the tests, but it meant to solicit early feedback on this approach.
Notably it only works for GitLab sync backends (though others will
revert to the default behaviour.)


A working example can be seen at: https://org.kindrobot.ca

This work in progress / proof-of-concept shows a potential solution for
links to files that are not org files or folders (200ok-ch#325). It does this by:

- saving the URL to the GitLab project at login time
- parsing links to files that contain a "." but are not org files
- making a link to the file with the template
  `${gitLablProjectUrl}/-/blob/${defaultBranch}/${path}`

The commit is not production ready and has not added or updated any of
the tests, but it meant to solicit early feedback on this approach.
Notably it _only_ works for GitLab sync backends (though others will
revert to the default behaviour.)
@branch14
Copy link
Member

branch14 commented Jul 6, 2022

Thank you @motevets for your pull request. Your contribution is highly appreciated. Your motivation and approach are comprehensible. I don't mean to be blunt, but I'll be frank since I don't want to raise false hopes that we will be going into that direction. Let me use your contribution as a good basis for a discussion on how to move forward.

I concur, it is unsatisfactory that linked files cannot be opened in organice. But organice, being a PWA, runs in a browser. It should be feasible to display most files without leaving its context.

Leaving the context of the PWA raises usability issues. What happens on the 6 different platforms[1] when opening an ExternalLink? Leaving the context of the PWA will also mean losing Authorization.

Before enhancing organice we try to answer the question WWED (What would Emacs do?) Whenever possible organice tries to replicate the behavior of Emacs in order not to violate the principle of least surprise. To answer the question: As Emacs will open local files by itself, this is what organice should do.

The current implementation, which identifies org files by there extensions (.org or .org_archive) and falls back to assuming everything else is a directory is a hack -- a hack with quite some history[2]. Tackling this topic, it might be helpful to be familiar with the history, as some valid points have already been discussed by bright minds.

Adding more conditions based on looking at the path or filename does not feel like a solid solution. IMO the only way to handle files (and directories) properly is to fetch it and deal with what is received. This might mean we have to convert raw image data into displayable blobs and maybe enhance the sync backend clients.

Organice's sync backend clients are implemented as a strategy pattern. The set of functions exposed by the modules is an interface that all clients must adhere to. Adding to this interface raises some concerns as divergence is a maintainability issue.

Again, I didn't mean to be blunt. I want to thank you for the time you put into this and I love to see the joined effort in improving organice's features and usability.

[1] The 6 platforms organice runs on

  • in a desktop browser
  • in a mobile browser
  • as PWA installed on android
  • as PWA installed on ios
  • webview in android app (soon to come)
  • webview in ios app (soon to come)

[2] History of organice's file browser

@kindrowboat
Copy link
Author

Thank you for your thoughtful response @branch14 . I had a feeling this would not be a generally acceptable solution. I implemented it in my fork, and it's suiting my needs, so in the spirit of open source, I thought I should share back my modifications for consideration, no matter how remote the chances of them getting merged in. I'll go ahead and close this PR.

I'd be interested in contributing a more cohesive and comprehensive PR down the road when I have the time, unless its on someone at 200ok's todo list.

@kindrowboat kindrowboat closed this Jul 6, 2022
@munen
Copy link
Collaborator

munen commented Jul 6, 2022

@motevets It's something that we have thought about in the back of heads since starting organice. So, if somebody beats us to the chase, we will be thankful for the contribution.

When you have time, and we haven't implemented this yet, we are happy to support you with guidance, code reviews and testing anytime 🙏

@kindrowboat
Copy link
Author

kindrowboat commented Jul 6, 2022

I was thinking about this some more and was wondering if this would be an acceptable solution based on what @branch14 said.

This might mean we have to convert raw image data into displayable blobs and maybe enhance the sync backend clients.

Since the sync backends already have the ability to sync data, would it be acceptalbe to make blobs from the raw data and trigger a download? (I help maintain a library that implements this kennethjiang/js-file-download.) I think that would work for all the different PWA use-cases, and it might be an itterative step towards displaying an image inside of organice (for say, inline images). (Side note: inline images aren't a big motivation for me. I'm more likely to have links to PDFs or other files I'll need while completing the task.

@branch14
Copy link
Member

branch14 commented Jul 7, 2022

@motevets Yes, triggering a download might be a good first step. It might even be enough for many file types.

I guess there will be a lot of edge cases that need to be dealt with, e.g. what if someone links to large files. Ideally the sync backends provide ways to query metadata, like mimetype and size, and organice can take suitable actions based on that.

But even for larger files a download might be the expected outcome. (Some anecdotal evidence: Decades ago I worked at an organization that had no limit on the size of email attachments and staff would regularly send ISO images and then complain to IT that their inboxes are full and everything is slow. Good times.)

If we had metadata we could issue a warning before starting the download.

@munen
Copy link
Collaborator

munen commented Jul 7, 2022

Adding to what @branch14 said: I think it could be quite easy to embed different kinds of filetypes into their respective tag:

So, PDFs are not an issue. And maybe 'other files' will be possible as well with this approach(;

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.

3 participants