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

enhance plugin update #508

Merged
merged 3 commits into from
Dec 23, 2024
Merged

enhance plugin update #508

merged 3 commits into from
Dec 23, 2024

Conversation

xxfttkx
Copy link
Contributor

@xxfttkx xxfttkx commented Dec 23, 2024

#507
优化规则更新
image

目前UI表现如上,且右上角增加了更新所有规则的按钮。

如果需要改成什么别的UI形式请告诉我。

@Predidit
Copy link
Owner

@xxfttkx

  1. 我们能否合并 canUpdatePlugin 和 inPluginHTTPList 实现一个类似 pluginStatus 的方法,不过传入的变量类型为 Plugin,这样结构清晰一些

  2. 既然我们现在实现了 tryUpdatePlugin 方法,我们也许可以再实现一个 tryInstallPlugin 方法,并将 /lib/page/plugin_editor 下的页面大部分涉及到 savePluginToJsonFile 的调用替换为以上两个方法

  3. 我建议在进行规则全量更新时使用 showLoading 而不是 showToast , 这样可以屏蔽更新时的其他操作,避免预期外的行为

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

@xxfttkx

  1. 我们能否合并 canUpdatePlugin 和 inPluginHTTPList 实现一个类似 pluginStatus 的方法,不过传入的变量类型为 Plugin,这样结构清晰一些
  2. 既然我们现在实现了 tryUpdatePlugin 方法,我们也许可以再实现一个 tryInstallPlugin 方法,并将 /lib/page/plugin_editor 下的页面大部分涉及到 savePluginToJsonFile 的调用替换为以上两个方法
  3. 我建议在进行规则全量更新时使用 showLoading 而不是 showToast , 这样可以屏蔽更新时的其他操作,避免预期外的行为

@Predidit

  1. 可以, 我稍后修改后提交
  2. 我稍后提交
  3. 我稍后修改后提交,但我不是很理解这两个方法的区别

@Predidit
Copy link
Owner

  1. showLoading 的全屏遮罩会阻止使用者进行其他操作,而 showToast 的提示没有办法阻止使用者在更新期间进行一些可能导致问题的操作 (例如手动更新某条规则)

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

  1. showLoading 的全屏遮罩会阻止使用者进行其他操作,而 showToast 的提示没有办法阻止使用者在更新期间进行一些可能导致问题的操作 (例如手动更新某条规则)

了解了,感谢解答

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

@Predidit
我是不是理解错了第2点,tryInstallPlugin内的具体实现需要是怎样的

@Predidit
Copy link
Owner

没有问题

不过像是 lib/pages/plugin_editor/plugin_shop_page.dart L136-146 这种场合,为什么也用 tryInstallPlugin 而不是 tryUpdatePlugin 啊,这样看上去很冗余

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

没有问题

不过像是 lib/pages/plugin_editor/plugin_shop_page.dart L136-146 这种场合,为什么也用 tryInstallPlugin 而不是 tryUpdatePlugin 啊,这样看上去很冗余

没注意实现直接直接把用到savePluginToJsonFile的改成tryInstallPlugin了,我现在再复查一遍再提交

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

@Predidit
有个问题

if (int.parse(pluginHTTPItem.api) > Api.apiLevel) {
  KazumiDialog.showToast(message: 'kazumi版本过低, 此规则不兼容当前版本');
  return;
}

这段中的showToast是必要的吗
tryUpdatePlugin中判断apiLevel不符合后返回false,无法定位到是版本过低的问题导致的导入失败

@Predidit
Copy link
Owner

可以在控制器层使用 KazumiDialog 相关调用

如果要只在页面上调用的话,也许我们可以修改 tryUpdatePlugin 的返回值为 int

以 int 来表示结果

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

可以在控制器层使用 KazumiDialog 相关调用

如果要只在页面上调用的话,也许我们可以修改 tryUpdatePlugin 的返回值为 int

以 int 来表示结果

好的,在控制器层调用KazumiDialog的话,返回值是void也行吧?int有什么额外的用处吗

另外,ib/pages/plugin_editor/plugin_shop_page.dart L136-146 的场合,tryUpdatePlugin似乎更适合传入string name作为参数,因为已有的是PluginHTTPItem并不是Plugin,需要在控制器层修改吗,可以重载吗。

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

@Predidit
我还是觉得不适合在tryUpdatePlugin里调用KazumiDialog,因为tryUpdateAllPlugin中有循环使用
tryUpdatePlugin,在这期间应该不需要任何Dialog输出。

@Predidit
Copy link
Owner

返回 int 来表示错误类型,例如用 -1 表示因为 apiLevel 导致的失败,0 表示其他原因的失败,1 表示成功

这样我们就可以根据结果直接在 Page 中调用 KazumiDialog

如果对代码质量有比较高的要求的话,我们可以使用重载,dart 也支持重载,我们还可以使用 enum 来让代码更有可读性

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

了解了,我来进行修改

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

尝试发现dart不支持重载,我想写个新方法tryUpdatePluginByName

@Predidit
Copy link
Owner

我好像搞混了一些东西。

实在不行可以将 tryUpdatePlugin 的入参调整为多个可选的参数,例如 {String? name, Plugin? plugin} 来实现类似重载的功能,不过看上去有点奇怪就是了。

这个模块设计的时候缺乏考虑,在现在这个PR中怎么修改都有些奇怪。

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Dec 23, 2024

提交了,感觉代码还行,就是用int判断各种情况不太好看。

想着去utils/constants.dart里写enum又觉得没必要,情况太少了。

感觉view层逻辑太多了,我是偏向在controller中创建很多函数的。

@Predidit
Copy link
Owner

看事情已经没有问题

感谢你的工作

@Predidit Predidit merged commit 6c71de6 into Predidit:main Dec 23, 2024
5 checks passed
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.

2 participants