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

#796: Add a button to sort files on "Manage your files" screen #946

Merged

Conversation

Jeepston
Copy link
Collaborator

Purpose

Requested enhancement to add sort option in Storage Management screen. Current sort options are Size (default), and Title. Further options can be added (full path, creation date etc.)

Related tasks

#796

Approach

Initially, rewrite entire screen to SwiftUI. Correspondently, update ViewModel, Coordinator and Tests. Delete StorageViewController, StorageTableViewCell and unused StoryBoard Scenes.

Then, add a sort button similar to that in Library, which shows Picker with sort options: Size (default), and Title. After selecting new option, ViewModel re-sorts files and stores the selection in UserDefaults.

Things to be aware of / Things to focus on

Translation for one of the sort option ("Size") is missing. There is a "TODO" comment in StorageView about it

Screenshots

Simulator Screenshot - iPhone 14 Pro - 2023-06-30 at 17 43 25

Copy link
Collaborator

@GianniCarlo GianniCarlo left a comment

Choose a reason for hiding this comment

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

There's an issue with the callbacks from the rows, I'm not getting the alert shown when tapping them (see video)

RPReplay_Final1688221466.MP4

Other than that and the details label sizes I mentioned in my other comment, this is a great step forward in the SwiftUI migration process 🥳 ! also, good call on making the delete and fix icons a bit bigger, they're much easier to tap now

.foregroundColor(themeViewModel.primaryColor)

Text(item.path)
.font(.subheadline)
Copy link
Collaborator

@GianniCarlo GianniCarlo Jul 1, 2023

Choose a reason for hiding this comment

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

I think this .subheadline and the next one, needs to be a bit smaller, to emulate what we have on the library cells

@Jeepston
Copy link
Collaborator Author

Jeepston commented Jul 1, 2023

Interesting 🤔 It worked initially. I’ll try to fix it tomorrow

@Jeepston
Copy link
Collaborator Author

Jeepston commented Jul 2, 2023

@GianniCarlo I've pushed the fix. In which I've learned that SwiftUI has some troubles handling multiple .alert modifiers which present Alert struct. Newer alert modifiers do not have this issue, but they are only available from iOS 15. So I've rewritten alert handling - there is only one modifier in the View, but it shows different Alerts depending on user action.

Finding number 2: SwiftUI defines some data type LibraryItem somewhere, so there are conflicts when using app's Core Data class LibraryItem in SwiftUI. It should be pre-pended with BookPlayerKit. 🙄

Finding number 3: when deleting files from Storage Management, it can leave "empty" LibraryItem in the DB, which shows in the Library, but cannot be played. I think this should be fixed, and I will submit it in the separate PR

@Jeepston Jeepston force-pushed the 796--sort-file-on-storage-screen branch from 97ecd4b to 8ee2884 Compare July 2, 2023 14:26
@Jeepston Jeepston force-pushed the 796--sort-file-on-storage-screen branch from 8ee2884 to 5bd7433 Compare July 2, 2023 15:15
@GianniCarlo
Copy link
Collaborator

  • Yeah it's sort of frustrating that modifiers are gated to newer iOS releases :/
  • I ran into something similar with the Section enum, I'll keep this in mind when I continue with the Realm migration, so I will probably add a prefix BP to the data models, thanks for the heads up
  • This one is by design, I think it was one of the requests I got, that they wanted to offload the files, and keep the progress, so they could potentially in the future import that book again, and it will remap to the object in the library

I'll take a look at the new changes in the morning, I got held up today by trying to figure out why Realm was not compiling, and it was because of the build setting APPLICATION_EXTENSION_API_ONLY set to true, I reported the issue on their repo, so hopefully there's a workaround to not disable that flag

@GianniCarlo GianniCarlo merged commit a848cdf into TortugaPower:develop Jul 3, 2023
@Jeepston Jeepston deleted the 796--sort-file-on-storage-screen branch July 4, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants