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

Refactor Setting - Service and provide service configuration view #326

Merged
merged 23 commits into from
Jan 17, 2024

Conversation

CanglongCl
Copy link
Collaborator

No description provided.

@CanglongCl
Copy link
Collaborator Author

#324
For service configuration, only a framework has been written, and the details of the configuration need to be filled in.

@tisfeng tisfeng requested a review from phlpsong January 15, 2024 02:29
@phlpsong
Copy link
Collaborator

phlpsong commented Jan 15, 2024

I tried this change, and it looks good!

Meanwhile, I've noticed some minor UI issues:

  1. The Toggle size increased.
  2. The behavior of the service cell highlights has changed; could you double-check it? (I saw that the list style changes, I'm not sure if we can retain the old behavior.)
  3. The Picker cannot be fully shown in the sidebar while using English.
  4. The Text for no selection and no configuration should have a secondary color for a better appearance.

@tisfeng
Copy link
Owner

tisfeng commented Jan 15, 2024

Yes, I'd like to see the UI of the service list to be as consistent as possible with the previous one if there are no special requirements, especially the display style of the Picker and cell, which are not as recognizable in comparison to the new UI as they were before.

image image

@tisfeng
Copy link
Owner

tisfeng commented Jan 15, 2024

The rewritten ServiceTab.swift looks cleaner and more structured, we can refer to this code when we write SwiftUI later.

@CanglongCl
Copy link
Collaborator Author

I tried this change, and it looks good!

Meanwhile, I've noticed some minor UI issues:

  1. The Toggle size increased.
  2. The behavior of the service cell highlights has changed; could you double-check it? (I saw that the list style changes, I'm not sure if we can retain the old behavior.)
  3. The Picker cannot be fully shown in the sidebar while using English.
  4. The Text for no selection and no configuration should have a secondary color for a better appearance.

For point 4, I don't this text style should be secondary. Usually, secondary mean disabled. It may confuse user.

@tisfeng How do you think?

@CanglongCl
Copy link
Collaborator Author

image

@phlpsong
Copy link
Collaborator

For point 4 I think you are right. Let's keep the current setting.

@tisfeng
Copy link
Owner

tisfeng commented Jan 15, 2024

The Text for no selection and no configuration should have a secondary color for a better appearance.

Currently, it looks good to me, do you have any good idea for this? @phlpsong

@phlpsong
Copy link
Collaborator

The Text for no selection and no configuration should have a secondary color for a better appearance.

Currently, it looks good to me, do you have any good idea for this? @phlpsong

目前这个效果对我来说看起来不错,对于这个,你有更好的想法吗?

No other issues, LGTM

@tisfeng
Copy link
Owner

tisfeng commented Jan 15, 2024

ok.

@tisfeng
Copy link
Owner

tisfeng commented Jan 15, 2024

I'll add the rest of the service configuration later in the evening when I have time.

@CanglongCl
Copy link
Collaborator Author

This feature is almost completed, it's recommended to approve & merge before start add other configurations.

@tisfeng tisfeng requested a review from Kyle-Ye January 15, 2024 12:02
@tisfeng
Copy link
Owner

tisfeng commented Jan 15, 2024

I tested this code with no problems.

The only issue is that switching tab multiple times may cause lag, but this seems to be an internal issue with the allServices: method, which I'll troubleshoot later.

@tisfeng
Copy link
Owner

tisfeng commented Jan 15, 2024

@CanglongCl This is a strange issue, I have tested it and found that when the program is first running, switching Tabs doesn't cause the problem, it seems that after running for a while, switching Tabs causes allServices: to be called multiple times, and the CPU usage is very high.

service-issue.mp4

@CanglongCl
Copy link
Collaborator Author

@CanglongCl This is a strange issue, I have tested it and found that when the program is first running, switching Tabs doesn't cause the problem, it seems that after running for a while, switching Tabs causes allServices: to be called multiple times, and the CPU usage is very high.

service-issue.mp4

I didn't reproduce the issue. But I did a small refactor so now only the windowType changes will trigger calling to allServices

@CanglongCl
Copy link
Collaborator Author

Last question, the list on the left side of the service page seems to have a weird double animation, could you take a look? @CanglongCl

double-animation.mp4

Fixed

@CanglongCl
Copy link
Collaborator Author

I have fixed this issue.
The problem is that every time the ServiceItemView is initialized, the set enabled method is called to post an update notification, and the current service page has so many ServiceItemView.
image

Thank you! It is really a weird behavior of Toggle in SwiftUI...

I figured out it. It is because I called improperly set enabled while initializing the wrapper.

@tisfeng
Copy link
Owner

tisfeng commented Jan 16, 2024

I tested the new code, it looks good to me 👍

phlpsong
phlpsong previously approved these changes Jan 16, 2024
Copy link
Collaborator

@phlpsong phlpsong left a comment

Choose a reason for hiding this comment

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

LGTM

@tisfeng tisfeng requested a review from phlpsong January 17, 2024 02:01
@tisfeng
Copy link
Owner

tisfeng commented Jan 17, 2024

Please review the code, if no problem, I will merge this PR tonight.

@phlpsong
Copy link
Collaborator

I tested the latest commit, looks good to me 🎉

@Jerry23011
Copy link
Collaborator

Jerry23011 commented Jan 17, 2024

The new UI is awesome, super excited to see it in production 🥳🙌

Just some notes on minor issues here:

  • In settings, the button appears to experience lag when transitioning from other tabs to General
  • In Services tab, the list seems to wear off along with the elements when scrolling to the top/bottom
截屏2024-01-16 19 14 20 截屏2024-01-16 19 14 35
  • Scrolling in General tab feels laggy on 120Hz display (may be a SwiftUI performance issue 🥲)
  • Settings window often fails to appear in the front (again, likely to be on SwiftUI's side 🥲🥲)
2024-01-16.19.24.03-1.mov

I suggest keeping these features on beta for a while, either until Apple fixes them or until we find workarounds.

@CanglongCl
Copy link
Collaborator Author

CanglongCl commented Jan 17, 2024

The new UI is awesome, super excited to see it in production 🥳🙌

Just some notes on minor issues here:

  • In settings, the button appears to experience lag when transitioning from other tabs to General
  • In Services tab, the list seems to wear off along with the elements when scrolling to the top/bottom

截屏2024-01-16 19 14 20 截屏2024-01-16 19 14 35

  • Scrolling in General tab feels laggy on 120Hz display (may be a SwiftUI performance issue 🥲)
  • Settings window often fails to appear in the front (again, likely to be on SwiftUI's side 🥲🥲)

2024-01-16.19.24.03-1.mov
I suggest keeping these features on beta for a while, either until Apple fixes them or until we find workarounds.

The new UI is awesome, super excited to see it in production 🥳🙌

Just some notes on minor issues here:

  • In settings, the button appears to experience lag when transitioning from other tabs to General
  • In Services tab, the list seems to wear off along with the elements when scrolling to the top/bottom

截屏2024-01-16 19 14 20 截屏2024-01-16 19 14 35

  • Scrolling in General tab feels laggy on 120Hz display (may be a SwiftUI performance issue 🥲)
  • Settings window often fails to appear in the front (again, likely to be on SwiftUI's side 🥲🥲)

2024-01-16.19.24.03-1.mov
I suggest keeping these features on beta for a while, either until Apple fixes them or until we find workarounds.

For point 1 & 4, I think it SwiftUI's issue.

For point 2, I think it is the default behavior of List view. It now indicates the end of scrolling. If it's a problem, I will take more time to fix it. @tisfeng

For point 4, I think we can find a way to fix it, but I'm not familiar to window management in SwiftUI. Maybe we should handle it in another PR.

@phlpsong
Copy link
Collaborator

For point 4 I think it's a SwiftUI issue, same problem here https://developer.apple.com/forums/thread/731628.

@tisfeng
Copy link
Owner

tisfeng commented Jan 17, 2024

@Jerry23011 You're very observant. 👍

For points 1 and 3, there does seem to be a slight issue, but it's not very noticeable perceptually, probably because there are more elements in the General Tab, not sure if it's a performance issue with SwiftUI.

For points 2 and 4, release mode performs much better compared to debug mode, seems strange.

For this kind of problem, which is not very serious, we can check it a little bit, and if it's a problem with SwiftUI itself, we can ignore it for now and leave it to Apple to fix it.

@tisfeng
Copy link
Owner

tisfeng commented Jan 17, 2024

@Jerry23011 You can open a new issue to record these problem, we can handle them in another PR.

If there are no more questions, I will merge this PR later.

@Jerry23011
Copy link
Collaborator

You can open a new issue to record these problem, we can handle them in another PR.

okok

@tisfeng tisfeng merged commit 8edb419 into tisfeng:dev Jan 17, 2024
5 checks passed
@CanglongCl CanglongCl deleted the service branch January 18, 2024 01:39
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.

4 participants