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

Revamp Settings Section: Complete Migration to Swift & SwiftUI #2826

Closed
wants to merge 49 commits into from

Conversation

adityagi02
Copy link
Contributor

@adityagi02 adityagi02 commented Mar 2, 2024

💡 Description

This pull request aims to overhaul the existing Settings section of the app, transitioning it from the old Objective-C code with XLForm package usage to modern Swift code utilizing SwiftUI.
The revamp covers the Settings screen along with its connected components such as SettingsAdvanced Screen, AutoUpload Screen, ChangeAutoUploadFileName screen, and associated small views.

This significant refactoring enhances maintainability, optimises performance of app, improves user experience, and aligns with modern iOS development standards.

📝 Checklist

  • Navigation between More & Settings Screens
  • Settings Screen
  • Several Sheet Views in Settings
  • Settings ViewModel + Testing
  • Auto Upload Screen
  • AutoUpload ViewModel + Testing
  • Advanced Screen
  • Advanced ViewModel + Testing
  • ChangeAutoUploadFileName Screen
  • ChangeAutoUploadFileName ViewModel + Testing
  • Other bugs fixes in Settings Section

Postponed Tasks🧠

These tasks requires hefty considerations, so I'll pick these functionalities afterwards.

  • Settings Update on User switch

Settings

  • Lock active YES/NO ⚠️
  • Lock no screen
  • E2EE

Auto Upload

  • Auto Upload Directory

Advanced

  • Capabilities

P.S.: I'll keep adding screenshots with every particular screen commit.

Requested Changes

  • Font Size (15)
  • Extra Spacing at Bottom of Screen
  • View Title for Capabilities Sceen
  • Lock: On/Off not working

@adityagi02
Copy link
Contributor Author

adityagi02 commented Mar 2, 2024

@marinofaggiana please keep on reviewing once you get time, as otherwise this will be a big chunk of code at last. I'll try to be punctual, except if I have university exams in between.

@marinofaggiana
Copy link
Member

Hi @adityagi02 welcome !!
I absolutely thank you for your great help, for any questions, reviews or anything else we are here for you.
Out of curiosity, what university are you attending?

Note.

  • please use DCO for signed your push
  • compatibility with keychain get/put calls must be maintained
  • for everything else you are free to indulge yourself as you wish

Thanks a lot !

ping @mpivchev

@adityagi02
Copy link
Contributor Author

adityagi02 commented Mar 3, 2024

Thank you for the warm welcome! I love Nextcloud & I'm glad to be contributing here.💙

I'm currently enrolled at the Indian Institute of Information Technology. It's a privilege to be part of an institution that requires passing one of the toughest STEM exams in the world, the JEE Mains & Advanced, for admission.

I'll definitely keep the points you mentioned in mind.

Thanks again !!

@adityagi02
Copy link
Contributor Author

adityagi02 commented Mar 4, 2024

New Settings Screen

ss

Some queries:

  • Firstly, In this Privacy Form, we have vaguely defined toggle text which is confusing. Either we have to change the content text or we have to add some proper footer explanation of what each toggles do.
  • When you click "Get Source Code", a browser sheet opens but it's UI is not proper, we have to follow a common and more descriptive view for this.✅
    Screenshot 2024-03-04 at 9 40 36 AM Screenshot 2024-03-05 at 1 27 08 PM

Here user cannot Sign In as cancel button is overlapping. Corrected✅

@marinofaggiana
Copy link
Member

marinofaggiana commented Mar 5, 2024

Hi @adityagi02 some problem with OpenSSL ?

Screenshot 2024-03-05 alle 10 07 52

@adityagi02

This comment was marked as resolved.

@marinofaggiana
Copy link
Member

Hi @adityagi02 some problem with OpenSSL ?
Screenshot 2024-03-05 alle 10 07 52

yeah, my Xcode which is causing some issue with fetching OpenSSL, so i kept a local dependency you can just add OpenSSL from manager and build with that it'll work fine.

yes, sure, but is strange, it shouldn't happen

@adityagi02

This comment was marked as resolved.

Signed-off-by: adityagi02 <[email protected]>
Signed-off-by: adityagi02 <[email protected]>
@adityagi02
Copy link
Contributor Author

New AutoUpload Screen
AUSR

Queries:

  • We have to rethink the arrangements of these sections, and reconsider these vaguely defined texts on them.

@marinofaggiana
Copy link
Member

marinofaggiana commented Mar 6, 2024

@adityagi02 thanks again for the work you are doing, remember that user switching also needs to be managed.

For example, you can be in the settings, push "Files" , change user and then push "More", at this point the setting must be reloaded otherwise it shows incorrect data (of the previous user).

@adityagi02
Copy link
Contributor Author

adityagi02 commented Mar 8, 2024

New Advanced Screen
advanced

Changes:

  1. All picker menus are now new SwiftUI pickers and constant throughout Settings.
    Old:-
    IMG_4466 IMG_4467
    _New:-_✅
    Screenshot 2024-03-08 at 11 48 36 AM Screenshot 2024-03-08 at 11 48 42 AM

  2. No need of any new View❌
    IMG_4468

  3. I have made the trash image red, so that it highlights.(Just my idea)

Queries:

  1. We have same texts for two different sections, should we have some changes on these.

Screenshot 2024-03-08 at 11 30 02 AM Screenshot 2024-03-08 at 11 30 11 AM

@mpivchev

@adityagi02
Copy link
Contributor Author

adityagi02 commented Mar 9, 2024

We were not having any confirmation alerts, when the user taps on clear log file section.

Added✅
Screenshot 2024-03-09 at 10 43 58 PM

@adityagi02
Copy link
Contributor Author

adityagi02 commented Mar 9, 2024

Sorry for the DCO, i can't sort this out. I've tried commiting from terminal and XCode both, still It's not working.

@mpivchev
Copy link
Collaborator

Hi @adityagi02, thank you for this, it looks great so far :)

@marinofaggiana
Copy link
Member

@adityagi02 can you rebase this PR thanks

@adityagi02
Copy link
Contributor Author

@adityagi02 can you rebase this PR thanks

Done ☑️

adityagi02

This comment was marked as resolved.

adityagi02

This comment was marked as resolved.

@marinofaggiana
Copy link
Member

Publishing changes from background threads is not allowed; make sure to publish values from the main thread (via operators like receive(on:)) on model updates.

@marinofaggiana
Copy link
Member

Bug: Button example [View log file] works only when touch the label

@marinofaggiana
Copy link
Member

marinofaggiana commented May 3, 2024

Bug: Disable crash reporter: do not present the message box and do not have effect

@marinofaggiana
Copy link
Member

Please verify if all switch works like the Master version. thanks

@marinofaggiana
Copy link
Member

Lock [on/off] do I have to complete it? I do not remember

Copy link
Member

@marinofaggiana marinofaggiana left a comment

Choose a reason for hiding this comment

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

read up

@marinofaggiana
Copy link
Member

@adityagi02 why you use text + extra_space_for_form_taps and do not use the Button ?

@adityagi02

This comment was marked as resolved.

@marinofaggiana
Copy link
Member

Note. there is some issue with then new Realm library version, please rebase. Thx

@adityagi02
Copy link
Contributor Author

Note. there is some issue with then new Realm library version, please rebase. Thx

ok, sure

Copy link
Contributor Author

@adityagi02 adityagi02 left a comment

Choose a reason for hiding this comment

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

Added all changes requested. Added common NCBrandColor.shared.iconImageColor as well.
Remains: Lock off/on section.

@marinofaggiana
Copy link
Member

@adityagi02 I start Lock [on/off] in your branch

@adityagi02
Copy link
Contributor Author

@adityagi02 I start Lock [on/off] in your branch

Great, thanks !!
Please ping me when you complete it. I'll go through it as well.
Also, in case you find any other bug while working in my branch, report it. I'll work on that after your task completion.
Thanks again!! 😃

@marinofaggiana
Copy link
Member

Moved here:

#2929

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants