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

Add service tab in settings use SwiftUI #311

Merged
merged 23 commits into from
Jan 14, 2024

Conversation

phlpsong
Copy link
Collaborator

@phlpsong phlpsong commented Jan 7, 2024

Summary

#288 Add a new service tab using SwiftUI

Screenshots and videos:

Screenshot 2024-01-07 at 10 32 04
Screen.Recording.2024-01-07.at.10.33.17.mov

Appreciate it if you could have a look at this PR and comments are welcomed.

@phlpsong
Copy link
Collaborator Author

phlpsong commented Jan 7, 2024

Forget to mention, that I did not find a similar icon for the tab in SF Symbols.

If you find a better one, please let me know.

@tisfeng
Copy link
Owner

tisfeng commented Jan 7, 2024

我试了一下,看起来不错。briefcase 这图标没问题。

发现开启服务会导致崩溃

-[__SwiftValue integerValue]: unrecognized selector sent to instance 0x6000035ac420
(
	0   CoreFoundation                      0x000000018fa76800 __exceptionPreprocess + 176
	1   libobjc.A.dylib                     0x000000018f56deb4 objc_exception_throw + 60
	2   CoreFoundation                      0x000000018fb283bc -[NSObject(NSObject) __retain_OA] + 0
	3   CoreFoundation                      0x000000018f9e0a84 ___forwarding___ + 1572
	4   CoreFoundation                      0x000000018f9e03a0 _CF_forwarding_prep_0 + 96
	5   Easydict-debug                      0x00000001049fc22c -[EZBaseQueryViewController handleServiceUpdate:] + 104
	6   CoreFoundation                      0x000000018f9f6820 __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 148
	7   CoreFoundation                      0x000000018fa8a8ec ___CFXRegistrationPost_block_invoke + 88
	8   CoreFoundation                      0x000000018fa8a834 _CFXRegistrationPost + 440
	9   CoreFoundation                      0x000000018f9c548c _CFXNotificationPost + 764
	10  Easydict-debug                      0x0000000104a91e10 $s8Easydict10ServiceTabV010postUpdateB12NotificationyyF + 964
	11  Easydict-debug                      0x0000000104a90710 $s8Easydict10ServiceTabV14serviceToggled5index8isEnableySi_SbtF + 748
	12  Easydict-debug                      0x0000000104a90414 $s8Easydict10ServiceTabV11serviceListQrvg7SwiftUI18DynamicViewContentPAEE6onMove7performQry10Foundation8IndexSetV_SitcSg_tFQOyAE7ForEachVySaySi6offset_So13EZServiceTypea7elementtGSiAA0b4ItemI0VG_Qo_yXEfU_AVSi_ARtcfU_ySbcfU_ + 72
	13  Easydict-debug                      0x0000000104a7b044 $s8Easydict15ServiceItemViewV4bodyQrvg7SwiftUI05TupleD0VyAE0D0PAEE5frame8minWidth05idealK003maxK00J6Height0lN00mN09alignmentQr12CoreGraphics7CGFloatVSg_A5uE9AlignmentVtFQOyAE5ImageV_Qo__AE4TextVAiEE11toggleStyleyQrqd__AE06ToggleW0Rd__lFQOyAE0X0VyAE05EmptyD0VG_AE06SwitchxW0VQo_tGyXEfU_ySbcfU_ + 92
	14  Easydict-debug                      0x0000000104aa9460 $s7SwiftUI7BindingV8EasydictE6didSet7executeACyxGyxc_tFyxcfU0_ + 316
	15  SwiftUI                             0x00000001bbaedaac OUTLINED_FUNCTION_24 + 3972
	16  SwiftUI                             0x00000001bb18bda8 OUTLINED_FUNCTION_0 + 7992
	17  SwiftUI                             0x00000001bb18a358 OUTLINED_FUNCTION_0 + 1256
	18  SwiftUI                             0x00000001bac17f20 OUTLINED_FUNCTION_13 + 4184
	19  SwiftUI                             0x00000001ba38d754 objectdestroyTm + 272
	20  SwiftUI                             0x00000001bb18bda8 OUTLINED_FUNCTION_0 + 7992
	21  SwiftUI                             0x00000001bb18a358 OUTLINED_FUNCTION_0 + 1256
	22  SwiftUI                             0x00000001bbaef6d8 objectdestroy.2Tm + 3400
	23  SwiftUI                             0x00000001bb18a358 OUTLINED_FUNCTION_0 + 1256
	24  SwiftUI                             0x00000001bbaef6d8 objectdestroy.2Tm + 3400
	25  SwiftUI                             0x00000001bb18a358 OUTLINED_FUNCTION_0 + 1256
	26  SwiftUI                             0x00000001bb18b788 OUTLINED_FUNCTION_0 + 6424
	27  SwiftUI                             0x00000001bb18bb30 OUTLINED_FUNCTION_0 + 7360
	28  SwiftUI                             0x00000001bb18a358 OUTLINED_FUNCTION_0 + 1256
	29  SwiftUI                             0x00000001bbaef6d8 objectdestroy.2Tm + 3400
	30  SwiftUI                             0x00000001bb18a358 OUTLINED_FUNCTION_0 + 1256
	31  SwiftUI                             0x00000001bbc8b5ac OUTLINED_FUNCTION_9 + 4024
	32  SwiftUI                             0x00000001bbc8b3f0 OUTLINED_FUNCTION_9 + 3580
	33  SwiftUI                             0x00000001bbc8b614 OUTLINED_FUNCTION_9 + 4128
	34  AppKit                              0x0000000193380464 -[NSApplication(NSResponder) sendAction:to:from:] + 460
	35  AppKit                              0x0000000193380268 -[NSControl sendAction:to:] + 72
	36  AppKit                              0x000000019337d4f0 NSControlTrackMouse + 1168
	37  AppKit                              0x000000019337c54c -[NSControl mouseDown:] + 844
	38  AppKit                              0x000000019337b18c -[NSWindow(NSEventRouting) _handleMouseDownEvent:isDelayedEvent:] + 3472
	39  AppKit                              0x0000000193306690 -[NSWindow(NSEventRouting) _reallySendEvent:isDelayedEvent:] + 364
	40  AppKit                              0x0000000193306350 -[NSWindow(NSEventRouting) sendEvent:] + 284
	41  AppKit                              0x00000001939aff30 -[NSApplication(NSEventRouting) sendEvent:] + 1604
	42  AppKit                              0x0000000193602110 -[NSApplication _handleEvent:] + 60
	43  AppKit                              0x00000001931ce124 -[NSApplication run] + 512
	44  AppKit                              0x00000001931a53cc NSApplicationMain + 880
	45  SwiftUI                             0x00000001ba30cf80 OUTLINED_FUNCTION_12 + 15824
	46  SwiftUI                             0x00000001bab9d30c OUTLINED_FUNCTION_3 + 196
	47  SwiftUI                             0x00000001bafe34f0 OUTLINED_FUNCTION_1 + 152
	48  Easydict-debug                      0x0000000104a7f1f4 $s8Easydict0A17CmpatibilityEntryO4mainyyFZ + 100
	49  Easydict-debug                      0x0000000104a7f304 $s8Easydict0A17CmpatibilityEntryO5$mainyyFZ + 12
	50  Easydict-debug                      0x0000000104a813c0 main + 12
	51  dyld                                0x000000018f5a90e0 start + 2360
)

@phlpsong
Copy link
Collaborator Author

phlpsong commented Jan 7, 2024

OK, will take a look later

@tisfeng
Copy link
Owner

tisfeng commented Jan 7, 2024

整体没什么问题,有些细节还需要改进:

  • 设置页服务这里拖动修改服务顺序,应该发一个通知,更新查询窗口的服务顺序。
  • 图标和 Toggle 有点显大了。
  • 服务页窗口的宽度是否可以调整?相比之前,目前看起来有点宽。后面应该会在右侧添加服务的 api key 设置选项,到时候需要宽一点。

重写设置页时,功能和 UI 尽量和之前保持一致。

@tisfeng
Copy link
Owner

tisfeng commented Jan 7, 2024

看了一下代码,有通知 postUpdateServiceNotification,是 onServiceItemMove 逻辑有问题。

@phlpsong phlpsong closed this Jan 7, 2024
@phlpsong phlpsong force-pushed the phlpsong/service_tab_swiftui branch from 16c94fc to c26d506 Compare January 7, 2024 05:03
@phlpsong phlpsong reopened this Jan 7, 2024
@phlpsong
Copy link
Collaborator Author

phlpsong commented Jan 7, 2024

Sorry, accidentally closed this.

1. 设置页服务这里拖动修改服务顺序,应该发一个通知,更新查询窗口的服务顺序。

Yes, something is wrong with the order check logic, fixed in the latest commit.

2. 图标和 Toggle 有点显大了。

Updated, please check.

  1. 服务页窗口的宽度是否可以调整?相比之前,目前看起来有点宽。后面应该会在右侧添加服务的 api key 设置选项,到时候需要宽一点。

Need further check

I can't reproduce the crash issue, but it may be caused by the userInfo value, which should fixed in the latest commit.

@tisfeng
Copy link
Owner

tisfeng commented Jan 7, 2024

现在跑起来没问题了。

@Kyle-Ye 帮忙 review 一下代码。

@tisfeng tisfeng requested a review from Kyle-Ye January 7, 2024 08:25
@phlpsong
Copy link
Collaborator Author

phlpsong commented Jan 7, 2024

@tisfeng I have changed the frame size of the service tab to 360*540, which will look like the below:

Screenshot 2024-01-07 at 17 09 33

Please let me know your thoughts.

@tisfeng
Copy link
Owner

tisfeng commented Jan 7, 2024

修改后的窗口大小没问题,这个可以添加动画吗,不然切换 tab 时看起来好突兀 🤔

@tisfeng
Copy link
Owner

tisfeng commented Jan 7, 2024

这里需要去掉分割线,添加一下选中效果,高度也需要增加一下。

@phlpsong
Copy link
Collaborator Author

phlpsong commented Jan 7, 2024

Official support for separator and highlight behavior is available from macOS 13. Need some time to find a solution for this.

@tisfeng
Copy link
Owner

tisfeng commented Jan 7, 2024

Official support for separator and highlight behavior is available from macOS 13. Need some time to find a solution for this.

问题不大,目前新的设置页和菜单都是 macOS 13+。

但是有个问题,目前应用是支持 macOS 11+,如果用户系统是 macOS 12,那会使用旧的设置页吗,我没看到相关的兼容代码。

@phlpsong
Copy link
Collaborator Author

phlpsong commented Jan 7, 2024

OK, I get it.

No, I didn't find such logic. Maybe I could add this logic in EasydictCmpatibilityEntry.

like below


if NewAppManager.shared.enable && isOS13Above {
       EasydictApp.main()
 } else {
       _ = NSApplicationMain(CommandLine.argc, CommandLine.unsafeArgv)
 }

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Jan 8, 2024

OK, I get it.

No, I didn't find such logic. Maybe I could add this logic in EasydictCmpatibilityEntry.

like below


if NewAppManager.shared.enable && isOS13Above {
       EasydictApp.main()
 } else {
       _ = NSApplicationMain(CommandLine.argc, CommandLine.unsafeArgv)
 }

-1 on this.

This is 2 separate thing.

One is AppKit lifecycle(entry point) vs SwiftUI lifecycle(entry point)
One is original AppKit Settings and new SwiftUI Settings.

IMO, we will certainly migrate from the old lifecycle to the new one once we are already since App is available on macOS 11+. However we may still use the old AppKit/ObjectiveC based Settings on older OS.

We need a NSViewControllerRepresentable to wrap the old Settings VC as the corresponding SwiftUI.Settings in SwiftUI.

This may not work and have some bug, if so we may consider just drop macOS 13- support.

@tisfeng
Copy link
Owner

tisfeng commented Jan 8, 2024

We need a NSViewControllerRepresentable to wrap the old Settings VC as the corresponding SwiftUI.Settings in SwiftUI.

I don't know much about this, you guys may need to follow up and deal with it later.

This may not work and have some bug, if so we may consider just drop macOS 13- support.

Try to see if macOS 13- can be supported first, and if that's not possible, upgrading to macOS 13+ is acceptable.

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Jan 8, 2024

@tisfeng Since I could not reproduce the crash issue, could you kindly help to test it on your device?

About the cell highlight changes, onTapGesture will make the onMove modifier not work which affects the drag/drop feature. Could you confirm the default behavior(use accent color) is acceptable?

  1. onTapGesture and onMove can exist together if you code properly.
  2. Why we need custom select logic here? The default toggle click action and a future feature of right click to edit is enough.

@phlpsong
Copy link
Collaborator Author

phlpsong commented Jan 8, 2024

@tisfeng Since I could not reproduce the crash issue, could you kindly help to test it on your device?
About the cell highlight changes, onTapGesture will make the onMove modifier not work which affects the drag/drop feature. Could you confirm the default behavior(use accent color) is acceptable?

1. `onTapGesture` and `onMove` can exist together if you code properly.

Same issue in https://stackoverflow.com/a/64194868, but the accepted answer not working.

2. Why we need custom select logic here? The default toggle click action and a future feature of right click to edit is enough.

This is to customize the highlight color of cell selection and try to keep the same behavior as the AppKit version.

.listRowBackground(selectedIndex == index ? Color("service_cell_highlight") : Color.clear)

I think the default behavior is acceptable.

@tisfeng
Copy link
Owner

tisfeng commented Jan 8, 2024

In addition to adding the click highlight background color that makes it impossible to drag the cell, another issue is that the highlight index should only work for the current window type, e.g. mini, and the selection effect should disappear when switching to a fixed window.

@phlpsong
Copy link
Collaborator Author

phlpsong commented Jan 8, 2024

In addition to adding the click highlight background color that makes it impossible to drag the cell, another issue is that the highlight index should only work for the current window type, e.g. mini, and the selection effect should disappear when switching to a fixed window.

Fixed in e72ef0d. Also correct the behaviors after drag&drop and toggle action.

@tisfeng
Copy link
Owner

tisfeng commented Jan 8, 2024

Good, these two bugs are both fixed.

@tisfeng tisfeng requested a review from Kyle-Ye January 8, 2024 16:51
@tisfeng
Copy link
Owner

tisfeng commented Jan 13, 2024

I tried the latest code and it looks like the problem is solved, is there anything else to add to this PR? @phlpsong

@phlpsong
Copy link
Collaborator Author

I tried the latest code and it looks like the problem is solved, is there anything else to add to this PR? @phlpsong

Yes, animation and selection issues have been fixed.

No more commits from my side.

@tisfeng
Copy link
Owner

tisfeng commented Jan 13, 2024

ok

@tisfeng tisfeng merged commit baca1fd into tisfeng:dev Jan 14, 2024
5 checks passed
@phlpsong phlpsong deleted the phlpsong/service_tab_swiftui branch January 22, 2024 03:01
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.

5 participants