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

Fix opening new tabs on macos(#518) #536

Merged
merged 26 commits into from
Nov 21, 2023
Merged

Fix opening new tabs on macos(#518) #536

merged 26 commits into from
Nov 21, 2023

Conversation

BPerlakiH
Copy link
Collaborator

@BPerlakiH BPerlakiH commented Nov 13, 2023

Fixes #518: Open link "in a new window" on macOS.
Partially based on #526

Known issues:

  • the opened tabs are not restoring their content on app restart, the tabID remains nil (might need another issue/ticket).
  • command+click on a link would not open it in a new tab (as I would expect it, since native safari has this capability)

@kelson42 kelson42 requested review from automactic and rgaudin and removed request for rgaudin November 13, 2023 22:00
@kelson42
Copy link
Contributor

@BPerlakiH Thank you.
@automactic Could you please have a look?

@kelson42
Copy link
Contributor

@BPerlakiH Please fix the two weaknesses you have listed so the PR is feature completed. Once done, please update PR comment and request new review.

@kelson42
Copy link
Contributor

@BPerlakiH Rebasing to benefit of the new CI

@automactic
Copy link
Member

@BPerlakiH Oops, I am so sorry I did not see you have this PR already opened for the external URL alert issue. So I made a PR (#539) and merged it, because I created the issue in the first place. Apologies for the merge conflict

As I have communicated with @kelson42, I won't have any capacity for PR reviews in the foreseeable future, so I'll give this one a quick look, and let him decide what to do

ViewModel/BrowserUIDelegate.swift Outdated Show resolved Hide resolved
ViewModel/BrowserViewModel.swift Outdated Show resolved Hide resolved
Views/BrowserTab.swift Outdated Show resolved Hide resolved
@kelson42
Copy link
Contributor

@automactic Thanks for having a quick look
@BPerlakiH Please fix the problem reported by @automactic and then we should be OK.

@BPerlakiH BPerlakiH self-assigned this Nov 20, 2023
@kelson42
Copy link
Contributor

@tvision251 I would like to have your review on this please.

Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

I can not judge the can and for the moment can not test it on my own. But if the points revealed by @automactic are fixed, then should be OK.

@kelson42 kelson42 mentioned this pull request Nov 20, 2023
1 task
@BPerlakiH BPerlakiH merged commit accc2d2 into kiwix:main Nov 21, 2023
1 of 3 checks passed
@BPerlakiH BPerlakiH deleted the fix/518-open-link-in-new-tab-on-macos branch November 21, 2023 08:16
@kelson42 kelson42 mentioned this pull request Nov 25, 2023
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.

Non functional "Open Link in New Window"
3 participants