Skip to content

Clean up code in InfoPane to be more robust#436

Merged
MattKiazyk merged 17 commits intoXcodesOrg:mainfrom
thai-d-v:fix-app-state
Nov 23, 2023
Merged

Clean up code in InfoPane to be more robust#436
MattKiazyk merged 17 commits intoXcodesOrg:mainfrom
thai-d-v:fix-app-state

Conversation

@thai-d-v
Copy link
Contributor

@thai-d-v thai-d-v commented Oct 14, 2023

Problem of InfoPane

  1. The preview code is too heavy. It crashes all the time.
  2. Explicit dependencies are too broad
    • make it hard to create preview data (in reality)
    • reduce performance (in theory)
    • reduce reusability (in the future)
  3. The body is too big
    • hard to reason
    • hard to test by preview

Approach

  1. I move subviews to separate files

    • reduce data dependency
    • reduce logic responsibilities, focus mostly on displaying data
    • reduce layout responsibilities. Let the outer view decide the layout
    • add preview to test locally

  2. I fix InforPane

    • reduce data dependency
    • improve preview performance. It is fast and doesn't crash anymore
    • fix layout
    • I move UnselectedView out to:
      • InfoPane should only care about displaying data, not whether an Xcode is selected

Discussion

  • I want to hear from you about this direction before I can go further to improve it.

@MattKiazyk
Copy link
Contributor

MattKiazyk commented Nov 22, 2023

Hi @thai-d-v

First of all thank you for taking the time to make Xcodes better!

I'm all for making InfoPane easier to read, easier to preview and all you've said above. This is a really good start.

The main question I have is around all the WrapperViews? What is the intent of those? I would rather see previews using the main view it's previewing vs having a wrapper.

Thanks again - let me know how I can help

MattKiazyk
MattKiazyk requested changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@MattKiazyk MattKiazyk left a comment

Choose a reason for hiding this comment

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

See comment - really awesome start - just questions around the WrapperView

@thai-d-v
Copy link
Contributor Author

thai-d-v commented Nov 22, 2023

Hi @MattKiazyk, WapperView provides a local interactive preview for InfoPane. It has some main benefits:

  1. faster preview
  2. local reasoning: we can preview interactively many states and layout of InfoPane with a small dependency

@MattKiazyk
Copy link
Contributor

MattKiazyk commented Nov 22, 2023

@thai-d-v thanks for the response. I see some benefit in it, however I would rather stick with the Apple way of using different previews for each view you are previewing. Added benefit would be that the view you are working on can be previewed as you are working on. In the WrapperView scenario, I would have to change the state PreviewName to be the one I'm on every time I wanted to edit and preview.

Feel free to use the #Preview macro now if you rebase from main. CI is now Xcode 15.

@thai-d-v
move release date view to ReleaseDateView.swift
7079e06 
    - I moved outside 2 lines of `.frame(maxWidth: .infinity, alignment: .leading)` because this layout job should belong to `InfoPane`
    - I added preview blocks for testing
@thai-d-v
move identical builds view to IdenticalBuildView.swift
e56d83d
@thai-d-v
move icon to IconView.swift
03eb166
@thai-d-v
move release notes to ReleaseNotesView.swift
f1fea63 
    - narrow down interfaces of some functions
@thai-d-v
move the compatibility view to CompatibilityView.swift
77ca8a2
@thai-d-v
remove openURL in InfoPane because ReleaseNotesView already has it
cecceea
@thai-d-v
rebase main
05f5c2c
@thai-d-v
rebase main
112829b
@thai-d-v
move and fix the UnselectedView
d08d2d3 
rebase main
@thai-d-v
move and fix the NotInstalledStateButtonsView
d172df6
@thai-d-v
fix layout of UnselectedView
9e787f9
@thai-d-v
move and fix the InstalledStateButtons
d406972
@thai-d-v
clean up code
4e55aac
@thai-d-v
fix data dependency, preview performance and layout out of InfoPane
a237958 
    - I move `UnselectedView` out to:
        1. narrow data dependency
        2. `InfoPane` should only care about displaying data, not whether an `Xcode` is selected
@thai-d-v
move the control section to InfoPaneControls, fix the layout to mak…
2f7d45c 
…e it self-contain
@thai-d-v
fix SDKView logic, hide the view when the content is empty
2fdc060
@thai-d-v
refactor preview code
a596e5f
@thai-d-v
Copy link
Contributor Author

thai-d-v commented Nov 23, 2023

@MattKiazyk You're right, code looks much cleaner with #Preview. I updated it.

@shakeliaj
Copy link

shakeliaj commented Nov 23, 2023 via email

@MattKiazyk MattKiazyk changed the title Fix InfoPane Clean up code in InfoPane to be more robust Nov 23, 2023
@MattKiazyk MattKiazyk added the chore Repo/Project upkeep label Nov 23, 2023
@MattKiazyk MattKiazyk merged commit b650261 into XcodesOrg:main Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MattKiazyk MattKiazyk MattKiazyk approved these changes

Assignees

No one assigned

Labels

chore Repo/Project upkeep

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thai-d-v @MattKiazyk @shakeliaj