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

Close Picture in Picture if the ZIM file is deleted/unlinked #1005

Merged
merged 29 commits into from
Nov 1, 2024

Conversation

BPerlakiH
Copy link
Collaborator

@BPerlakiH BPerlakiH commented Oct 12, 2024

Fixes: #1003

What has changed:

  • NavigationView updated to NavigationStack, as per Apple's recommendations
  • class functions changed to static. They are essentially the same, it's more of a lint issue
  • SwiftUI @State var were changed to private scope, it's also recommended (not to accidentally change it from the outside)
  • NavigationItem's .onBoarding renamed to .welcomeScreen to be inline with the name of the view where it navigates to
  • Fixed a couple of @MainActor (concurrency) issues

iOS:

  • Fixed issues with deleting tabs, there was a possibility to end up without any tabs on iPad (if the deletion of the last tab happened while something else was selected):
no_tabs_issue.mov
  • On unlinking / deleting a ZIM file, all the related tabs are closed, the Tabs entities are deleted, and the cached BrowserViewModels are removed, which removes the relevant WKWebViews. The Picture in Picture video started from the closing tab is suspended and stoped.
  • Note: iOS (both iPhone and iPad) works in a way that we have the "illusion" of tabs, we only really display 1 tab at a time. The surrounding views of the WKWebView are re-used / re-drawn when switching tabs. SwiftUI is designed this way, which in itself is not bad. The problem is that re-creating the WKWebView is very slow (I tested it), and noticeably bad for the user. For this reason we do cache the WKWebView itself, and some of its baggage in the form of BrowserViewModel. Taking the whole view out of cache and inserting into our redrawn Content is way more quicker.
  • There's another issue with WKWebView, it's almost impossible to "reset" it, and get it back to its original state. Once I was close to achieve it, but the web history (back forward buttons) cannot be edited (we met this problem before).
  • So the solution here is to delete the tab, and the cached BrowserViewModel, and create a new one (if needed).

MacOS

This was a harder case. Here we do not have the "illusion of tabs", we do have them in full instance named windows. In practice we can display them even side by side. From the perspective of the code: all the views and related models are duplicated (almost a full copy of the application) for each window / tab. Anything that is sent via Notification centre (eg. open url), will be sent to all windows. Solution here is to either identify who sent it, or filter the receiver. Anything that is static instance, will be shared amongst the windows / tabs.
Unfortunately SwiftUI is not giving easy access to the current window, or from the window the individual views or view models.

  • The former implementation was a bit of mix, we had the "reading" state for the current window, but the WKWebView was tied to the window, meaning it was created once, never to be released. This made the iOS tab solution only partially working, since there was no way to either "recreate" the WKWebView, or to clear it out completely.
  • Solution: I reused the same method of BrowserViewModel caching from iOS, with a bit of extra static workaround for the case when we want to open a url in a new window / tab. Since in this case we need to "link together" the newly created tab's id (which is also used a cache key for BrowserViewModel), and the window itself, so we know what tab id we should use in the new window (and application instance) for getting the BrowserViewModel.

Follow ups:

  • the macOS window restoration never really worked, and that is still not working at the moment, although after all these changes (and knowledge gained) it will be easier to achieve it, if needed. It's a rather low priority issue, as the only way to achieve it is to have something open and CMD+Q the app. If all the windows will be closed, one by one, we get back to the "initial" one window mode after relaunching the app.
  • even if the tab is closed/deleted and the related BrowserViewModel is cleared from the cache, it stays around in the memory (a leak). One of the reasons, I have found so far, is that it's easy to put a class into Swift UI's @Environment, but there's no easy way to take it out from there (so far I haven't found a solution for that).

@BPerlakiH BPerlakiH linked an issue Oct 12, 2024 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2024

Codecov Report

Attention: Patch coverage is 30.37975% with 165 lines in your changes missing coverage. Please review.

Project coverage is 39.02%. Comparing base (7a6425c) to head (baada11).

Files with missing lines Patch % Lines
ViewModel/BrowserViewModel.swift 4.05% 71 Missing ⚠️
App/App_macOS.swift 39.39% 20 Missing ⚠️
App/SidebarViewController.swift 34.48% 19 Missing ⚠️
SwiftUI/Model/LibraryOperations.swift 0.00% 18 Missing ⚠️
ViewModel/NavigationViewModel.swift 48.27% 15 Missing ⚠️
App/SplitViewController.swift 0.00% 4 Missing ⚠️
Model/ZimFileService/ZimFileService.swift 0.00% 4 Missing ⚠️
SwiftUI/Patches.swift 0.00% 4 Missing ⚠️
Model/Utilities/URL.swift 0.00% 3 Missing ⚠️
Views/Buttons/TabsManagerButton.swift 0.00% 2 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1005      +/-   ##
==========================================
+ Coverage   38.26%   39.02%   +0.75%     
==========================================
  Files         118      118              
  Lines        6883     6949      +66     
==========================================
+ Hits         2634     2712      +78     
+ Misses       4249     4237      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BPerlakiH BPerlakiH force-pushed the 1003-deleting-zim-file-should-stop-playing-pip-video branch from ea30db8 to 5f67f92 Compare October 18, 2024 10:20
@BPerlakiH BPerlakiH force-pushed the 1003-deleting-zim-file-should-stop-playing-pip-video branch from 5f67f92 to 2fc1b4f Compare October 19, 2024 08:36
@kelson42
Copy link
Contributor

@BPerlakiH Any news here?

@kelson42 kelson42 force-pushed the 1003-deleting-zim-file-should-stop-playing-pip-video branch from ae3b8f8 to baada11 Compare November 1, 2024 05:43
@kelson42 kelson42 merged commit f76e171 into main Nov 1, 2024
4 checks passed
@kelson42 kelson42 deleted the 1003-deleting-zim-file-should-stop-playing-pip-video branch November 1, 2024 10:15
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.

Deleting ZIM file should stop playing pip video
3 participants