Skip to content

Conversation

@ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Apr 11, 2024

☑️ Resolves

@nextcloud/router cannot be used as it is, because it relies on window.location which is different on Desktop app.

Before

  • For functions returning relative link, window._oc_webroot was replaced with an absolute base path instead of relative (https://nextcloud.ltd/cloud instead of /cloud) to return an absolute link
  • For functions returning an absolute link, a complete copy-pasting solution was made

It makes it impossible to work with relative links. And it is just not clean

getBaseUrl was literally the same as getRootUrl.

After

_oc_webroot is now kept original (relative) and only when needed temporally replaced with an absolute.

Added OCA.Talk.Desktop.runWithAbsoluteWebroot helper to run another function having absolute webroot.

@nextcloud/router is now patched with this helper and re-defined baseURL default value for simplicity, keeping the original implementation.

🖼️ Screenshots

Support for relative links in Talk by new autolink in NcRichText

🏚️ Before 🏡 After
relative-links-before relative-links-after

Also works with absolute links, but only if there is no base (webroot).

links

Signed-off-by: Grigorii K. Shartsev <[email protected]>
@ShGKme ShGKme added bug Something isn't working 3. to review labels Apr 11, 2024
@ShGKme ShGKme requested review from Antreesy and DorraJaouad April 11, 2024 15:43
@ShGKme ShGKme self-assigned this Apr 11, 2024
This allows:
- Working with relative links
- Have less custom code in the patcher

Signed-off-by: Grigorii K. Shartsev <[email protected]>
@ShGKme ShGKme force-pushed the fix/nextcloud-router-patching-with-origin-oc_webroot branch from 2392b1e to 99666eb Compare April 11, 2024 15:48
@ShGKme ShGKme requested a review from nickvergessen April 12, 2024 09:56
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

🦭

Looks valid, but didn't test. Can give it a try, if you describe edge cases that don't work with current app-handling (without PR)

@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 12, 2024

Can give it a try, if you describe edge cases that don't work with current app-handling (without PR)

Currently it can be tested with relative links like [/call/<token>](/call/<token>). See Before/after in screenshots.

But the most important part is that it makes getBaseUrl and getRootUrl works the same as on Web.
Before on Talk Desktop it was like getBaseUrl === getRootUrl, both were absolute.

New approach of patching allows to have much less custom behavior on Talk Desktop, less custom code in the patcher and in the end - to get rid of custom link handlers like #569 (see the last screenshot. It works without the closed PR).

Then with nextcloud-libraries/nextcloud-vue#5487 it will cover all the cases. (Currently it misses only links with non-empty webroot).

@ShGKme ShGKme merged commit 46dec1d into main Apr 12, 2024
@delete-merged-branch delete-merged-branch bot deleted the fix/nextcloud-router-patching-with-origin-oc_webroot branch April 12, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants