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 plugin drag feature #593

Merged
merged 6 commits into from
Jan 13, 2025
Merged

add plugin drag feature #593

merged 6 commits into from
Jan 13, 2025

Conversation

xxfttkx
Copy link
Contributor

@xxfttkx xxfttkx commented Jan 12, 2025

#143 相关。

添加规则拖拽排序,可自由拖拽排序并应用在info_page中。重写了规则保存逻辑。

@AoEiuV020 和sortByName有一定重复所以删掉了,如果需要时间相关信息的话可以在updatePlugin中写,我感觉没必要就没写。
image

image

@Predidit
Copy link
Owner

这个 PR 有些杂乱

  1. 部分函数的语义不清晰,例如 plugin_controller 中的 loadPlugins 方法,它返回了 plugin 数组,实质的加载过程却在该函数内部进行,这很奇怪。

  2. 这个 PR几乎没有注释,这让 1 中的问题更严重了。

  3. plugin_controller L177为什么要用这种奇怪的方法触发更新,这不是 mobx 正常的工作方式。

  4. 你测试过全新 kazumi 吗,我怀疑这个 PR 把默认规则的加载过程损坏了。

  5. 拖拽按钮的位置不美观,我们也许可以为其实现编辑按钮,至少要留出适当的边距。

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 12, 2025

  1. loadPlugins是之前的加载方式,更改返回值为List<String>只是方便通过它获得需要删除的规则文件
  2. 白天我来多添加些注释。不过我感觉我用函数名表示内容还算清晰的,有函数名和内容不符的我也加上了注释。有哪些语义不清晰的函数名例子吗
  3. updatePlugin时没能触发ui更新,所以写了这句
  4. 试过把规则文件夹内文件全删了,能正常工作的
  5. ui方面我不太了解,等白天我来研究一下

@Predidit
Copy link
Owner

  1. 如果改成了这样的逻辑,就需要考虑拆分这个函数,而不是在上面继续堆叠逻辑。

  2. loadPlugins 和 loadAllPlugins 这种命名同时出现在一个文件中真的很清晰吗。

  3. 是不是忘记了 Observe 组件,也许可以看一下 mobx 文档。

  4. 我的意思是全新安装 kazumi,我们有一个从程序目录下拷贝规则到应用数据目录下的逻辑,我怀疑这个PR会让全新安装的 kazumi无法读取默认规则。

  5. 期待你的改进。

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 12, 2025

  1. 想着之前的代码能直接拿来用就没改太多,看来的确是该拆分以让现在的逻辑更清晰。
  2. 应该不是Observe 组件的问题,如果没写组件这行代码同样不会生效,不过确实应该找找更正常的写法
  3. 之前和现在的逻辑都是本地没有任何规则时弹出免责申明,然后通过copyPluginsToExternalDirectory去获取默认规则。这部分我改了且试了是能正常运行的。

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 13, 2025

目前表现是这样
image

ObservableList相关的问题发现是pluginList[i] = plugin;没法触发ui更新。替换成pluginList.replaceRange(i, i + 1, [plugin]);行了,不过发现项目中也没这种写法,能接受吗。

@AoEiuV020
Copy link
Contributor

或许应该重新考虑一下真正需要的是"拖拽"还是"置顶",
后者可以算是排序规则的一部分,而且这也是聚合类app常见的功能了,
前者变动太大了,影响太多,而且使用起来真的方便吗,真的有需要把某个规则拖到某个位置的需求吗,

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 13, 2025

我认为有必要,拖拽能带来很高的自由度。目前在添加了很多规则的情况下,info_page中定位到自己更倾向的源并不方便

@Predidit
Copy link
Owner

@xxfttkx

  1. pluginList.replaceRange 完全没有问题,不过 plugin_controller.dart L52 同样需要修改
  2. 我们需要在 plugin_controller.dart 中添加注释,说明在 1.5.1 之后规则文件储存在 plugin.json,之前的储存方式在各规则分离,从下面的版本更新会将分离的规则合并为单一 plugin.json

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 13, 2025

@Predidit
注释这样写行吗

L52是把List转成ObservableList,应该不需要修改吧

@Predidit
Copy link
Owner

这里可以使用clear 之后 addAll 来触发更新,而不是这样直接赋值

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 13, 2025

感谢告知,已修改

@Predidit
Copy link
Owner

这里不需要先 clear 吗

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 13, 2025

一进入loadAllPlugins就clear了,不过现在的逻辑是只有初次进入时loadAllPlugins,save后并没有重新load,或许开始的clear也不需要。

@Predidit
Copy link
Owner

这个PR在我看来已经没有问题

感谢你的工作

@Predidit Predidit merged commit 1b1c087 into Predidit:main Jan 13, 2025
6 checks passed
@Predidit
Copy link
Owner

@xxfttkx

突然意识到这个 PR 破坏了向前兼容性

我们可以假设在运行完新版本,完成 plugin.json 的合并之后运行之前的版本

那么对 plugin.json 的解析将出现错误

我们应该修改 plugin.json 的储存位置,例如将其存放在现有位置的子文件夹下,以防止其被之前的版本错误解析

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 13, 2025

@Predidit
是的,plugins.json被尝试解析成Plugin时将会出错,我没想到这一点。

或许可以把plugins.json直接放在getApplicationSupportDirectory()下?如果放在/plugins/plugins中总感觉很奇怪。

@Predidit
Copy link
Owner

可以放在 /plugins/v2

@xxfttkx
Copy link
Contributor Author

xxfttkx commented Jan 13, 2025

ok,稍后提交新的pr

@xxfttkx xxfttkx mentioned this pull request Jan 13, 2025
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.

3 participants