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

Fix #683 (bangumi cards text overflow) #686

Merged
merged 15 commits into from
Feb 6, 2025
Merged

Conversation

garylkz
Copy link
Contributor

@garylkz garylkz commented Feb 1, 2025

#683 裁剪掉溢出的部分

image

image

@Predidit
Copy link
Owner

Predidit commented Feb 1, 2025

ClipRect 的性能开销很高,而且边缘过渡不平滑

啊,突然后悔了,我在思考这样做的必要

我们可以使用限制行数的做法,例如在 BangumiCard 上限制行数为2

移除历史记录卡片上的番剧标签等

@garylkz
Copy link
Contributor Author

garylkz commented Feb 1, 2025

我们可以使用限制行数的做法,例如在 BangumiCard 上限制行数为2

嗯,确实可行 (e.g. 2倍大小)

image

但一般情况下感觉有点浪费空间 (尤其是单行的)

image

要不干脆SingleChildScrollView就行了?

@Predidit
Copy link
Owner

Predidit commented Feb 1, 2025

滚动列表中嵌套滚动列表从来不是好主意

@garylkz
Copy link
Contributor Author

garylkz commented Feb 1, 2025

又或者用 textScaler: const TextScaler.linear(1.1) 来强制字体大小?

@Predidit
Copy link
Owner

Predidit commented Feb 1, 2025

这应该是更好的解决方法,在可能溢出的地方使用。

@garylkz
Copy link
Contributor Author

garylkz commented Feb 1, 2025

已经把 ClipRect 替换成使用 TextScaler

@Predidit
Copy link
Owner

Predidit commented Feb 1, 2025

我们进行其他更改的目的是什么。我们似乎移除了一些会让视觉效果变得更好点占位符。

@garylkz
Copy link
Contributor Author

garylkz commented Feb 2, 2025

啊,应该是试图修复OverflowBox无视占位时不小心移除的

@garylkz
Copy link
Contributor Author

garylkz commented Feb 2, 2025

我们进行其他更改的目的是什么。

a1. lib/bean/card/bangumi_card.dart:102 更改后的版本:

Column(
  crossAxisAlignment: CrossAxisAlignment.stretch,
  children: [
    Text(...),
    ...
  ],
),

可以用更少的实现原版的视觉效果

Column(
  crossAxisAlignment: CrossAxisAlignment.start,
  children: [
    Row(
      children: [
        Expanded(child: Text(...)),
      ]
    ),
    ...
  ],
),

我们似乎移除了一些会让视觉效果变得更好点占位符。

因为之前试图用 OverflowBox 裁减时发现 SizedBox 会被无视(原理我也不太清楚),所以索性把 SizedBox 去除,并加在 bangumi_card.dart:99 的padding里,之后再用 ClipRect 来裁减。不过后来被 TextScaler 取代但忘了加回 =_=

不过话说回来(参考 a1),根据我的观察(bangumi_card.dart:102)的 Column 是为了占位用途。既然 SizedBox 可以直接加入 Padding, 那除了

Padding(
  padding: Padding.fromLTRB(3, 4, 3, 0), 
  child: Column(
    children: [
      Text(...), 
      SizedBox(height: 1)
    ],
  ),
),

以外,有没有考虑直接去除 Column ?感觉这样会比较方便阅读理解+更改

Padding(
  padding: Padding.fromLTRB(3, 4, 3, 1),
  Text(...),
),

PS: bangumi_history_card 的那个 SizedBox 会被加回去

@Predidit
Copy link
Owner

Predidit commented Feb 2, 2025

相当不错的修改,我们确实应该那么做

@garylkz
Copy link
Contributor Author

garylkz commented Feb 2, 2025

好的已经修改完毕,没问题的话 #683 应该暂时解决了。

PS: 额,bangumi_card 忘了 alignment 。
PSS: 额。。。在 1.0 text scaling 的情况下 bangumi_history_card 还是有 overflow 的问题
Screenshot From 2025-02-02 18-39-43

@Predidit
Copy link
Owner

Predidit commented Feb 2, 2025

字体缩放本来就不是我们应该考虑的事情

字体缩放导致的溢出完全是预期内的

不过我觉得很奇怪,如果默认倍率是 1 的话,字体也太大了,正常情况下 1 不会溢出

直觉上这里可能有更复杂的问题,涉及 flutter 的物理像素和逻辑像素。头痛

@garylkz
Copy link
Contributor Author

garylkz commented Feb 2, 2025

直觉上这里可能有更复杂的问题,涉及 flutter 的物理像素和逻辑像素。头痛

嗯,确实。

不过我觉得很奇怪,如果默认倍率是 1 的话,字体也太大了,正常情况下 1 不会溢出

结果还是屏幕分辨率间接造成的吧

@garylkz
Copy link
Contributor Author

garylkz commented Feb 2, 2025

不过这会不会和history_page.dart:127有关?mainAxisExtent: 150

@Predidit
Copy link
Owner

Predidit commented Feb 2, 2025

我不清楚,可能和DPI缩放更相关。

@bggRGjQaUbCoE
Copy link
Contributor

字体缩放本来就不是我们应该考虑的事情

字体缩放导致的溢出完全是预期内的

不过我觉得很奇怪,如果默认倍率是 1 的话,字体也太大了,正常情况下 1 不会溢出

直觉上这里可能有更复杂的问题,涉及 flutter 的物理像素和逻辑像素。头痛

卡片高度可以用 MediaQuery.of(context).textScaler.scale( )

@garylkz
Copy link
Contributor Author

garylkz commented Feb 3, 2025

卡片高度可以用 MediaQuery.of(context).textScaler.scale()

ts.scale 不是用于字体大小的吗?

@bggRGjQaUbCoE
Copy link
Contributor

卡片高度可以用 MediaQuery.of(context).textScaler.scale()

ts.scale 不是用于字体大小的吗?

MediaQuery.of(context).textScaler.scale(value) 是根据 textScaler 计算的大小

@bggRGjQaUbCoE
Copy link
Contributor

好的已经修改完毕,没问题的话 #683 应该暂时解决了。

PS: 额,bangumi_card 忘了 alignment 。 PSS: 额。。。在 1.0 text scaling 的情况下 bangumi_history_card 还是有 overflow 的问题 Screenshot From 2025-02-02 18-39-43

或者考虑 FittedBox

@garylkz
Copy link
Contributor Author

garylkz commented Feb 3, 2025

或者考虑 FittedBox

嗯,如果搞定不了卡片高度的话可以试试,再不然就:

BangumiCard 上限制行数为2

移除历史记录卡片上的番剧标签等

@garylkz
Copy link
Contributor Author

garylkz commented Feb 3, 2025

卡片高度可以用 MediaQuery.of(context).textScaler.scale( )

刚刚试了,似乎不可行。不过参考了 popular_page.dart:261 经过一些修改后好像可以避免在 1.0 倍率的情况下溢出(试了 16:9 Linux 和 20:9 Android)

Android 平台:

Screenshot From 2025-02-03 23-46-07

Screenshot From 2025-02-03 23-46-19

Screenshot From 2025-02-03 23-46-33

PS:

    final tt = Theme.of(context).textTheme;
    final tsFontSize = tt.titleMedium?.fontSize ?? kDefaultFontSize;
    final lmFontSize = tt.labelMedium?.fontSize ?? kDefaultFontSize;

桌面平台:

Screenshot From 2025-02-03 23-50-21

Screenshot From 2025-02-03 23-50-08

Screenshot From 2025-02-03 23-50-00

@garylkz
Copy link
Contributor Author

garylkz commented Feb 5, 2025

刚进行了一些测试,桌面平台大致上是没问题了,但是如果窗体宽度太小还是会造成溢出

image

至于手机平台我就真的不理解了,明明都是一样的代码为什么跑到手机平台上会莫名出现这空位

image

@ErBWs
Copy link
Contributor

ErBWs commented Feb 5, 2025

Wrap 在手机上和电脑上的垂直方向上的间距行为是不一致的,手机上的 runSpacing 需要给 0

@garylkz
Copy link
Contributor Author

garylkz commented Feb 5, 2025

Wrap 在手机上和电脑上的垂直方向上的间距行为是不一致的,手机上的 runSpacing 需要给 0

嗯,大概调整至 -10 后问题被解决了,至于窗体宽度溢出的问题我再想想办法,毕竟 SliverGrid 有些难搞

- reverted from using Wrap to Column for predictability
- implemented `Utils.getTextPxHeight`
@garylkz
Copy link
Contributor Author

garylkz commented Feb 5, 2025

卡片高度可以用 MediaQuery.of(context).textScaler.scale()

参考了 https://api.flutter.dev/flutter/painting/TextStyle/height.html ,算是解决了溢出的问题,不过是使用回之前的 Column + Text, 因为 Wrap + Chip 的换行有些难以预测(还是要用 Column + Chip?)。

@Predidit
Copy link
Owner

Predidit commented Feb 6, 2025

额,我觉得上面的实现很 hack 。我不知道 @ErBWs 对此怎么看

我更喜欢 Chip 的方案,以及需要使用 colorscheme 中预调好的颜色,而不是自己自定义,自定义的颜色看上去非常不协调

@Predidit
Copy link
Owner

Predidit commented Feb 6, 2025

以及 class _PropertyText extends StatelessWidget 这种写法没有什么必要,为什么不简单地使用类似 Widget PropertyText() {retrun Text();} 的写法

既然已经用下划线作为变量名的开头来表示这是一个私有而非公有的对象,为什么不直接放到类的内部呢

@ErBWs
Copy link
Contributor

ErBWs commented Feb 6, 2025

image

我感觉这个其实就还不错

具体实现方式我没看代码,现在这种颜色和整体的 material 3 配色不是很搭,而且显得很大,完全可以统一颜色

番剧标签确实不需要保留,基本不会有番剧以外的标签出现

@garylkz
Copy link
Contributor Author

garylkz commented Feb 6, 2025

我更喜欢 Chip 的方案,以及需要使用 colorscheme 中预调好的颜色,而不是自己自定义,自定义的颜色看上去非常不协调

老实说我也比较喜欢 Chip,但是如果改为 Wrap 的话确实很难避免溢出的问题,除非你不介意做得像Trello一样的非对称网格大小,不过感觉工作量会有点大:

image

至于颜色的话,暂时不确定要用colorScheme中的哪个,可以再讨论。

既然已经用下划线作为变量名的开头来表示这是一个私有而非公有的对象,为什么不直接放到类的内部呢

一开始是担心代码会太长太复杂,不过现在看来确实没什么必要,可以直接放进内部。

具体实现方式我没看代码,现在这种颜色和整体的 material 3 配色不是很搭,而且显得很大,完全可以统一颜色

你是指五颜六色那个还是黑色那个显得很大?

@ErBWs
Copy link
Contributor

ErBWs commented Feb 6, 2025

你是指五颜六色那个还是黑色那个显得很大?

五颜六色那个

@garylkz
Copy link
Contributor Author

garylkz commented Feb 6, 2025

五颜六色那个

(其实这两种 Chip 除了 cornerRadius以外,其他参数完全一致)

- simplified history page `mainAxisExtent` height formula
- merged `_PropertyText`, `_PropertyChip` into `BangumiHistoryCardV.propertyChip`
@garylkz
Copy link
Contributor Author

garylkz commented Feb 6, 2025

已经改用 Wrap + Chip 了,顺便简化了 mainAxisExtent 的高度计算。现在的话大概长这样:

image

至于颜色方面有什么看法吗? 没记错的话 Chip 默认颜色好像是 surfaceCard 用的是 surfaceContainerLow

image

Screenshot From 2025-02-06 20-22-53

@Predidit
Copy link
Owner

Predidit commented Feb 6, 2025

这里 Surface 的观感有些奇怪

参考 https://m3.material.io/styles/color/choosing-a-scheme

其实可以考虑用 SecondaryContainer 之类的颜色,具体可以再看看,亮色模式下也不能太糟糕

除此之外的两个问题

  1. 看上去 getTextPxHeight 工具函数已经不再使用,不删掉它吗
  2. 为什么 propertyChip 传入的 value 是 Object 类型而不是 String 类型

@garylkz
Copy link
Contributor Author

garylkz commented Feb 6, 2025

  1. 看上去 getTextPxHeight 工具函数已经不再使用,不删掉它吗

啊,差点忘了

  1. 为什么 propertyChip 传入的 value 是 Object 类型而不是 String 类型

毕竟有些参数的类型是 int, DateTime 等类型, 感觉直接在内部转换成 String 会比较方便

@Predidit
Copy link
Owner

Predidit commented Feb 6, 2025

传入 String ,在外部完成转换

@ErBWs
Copy link
Contributor

ErBWs commented Feb 6, 2025

可以考虑用 Chip,FilterChip 会导致按到标签而不打开视频

其实可以考虑用 SecondaryContainer 之类的颜色

历史记录卡片颜色就是 secondaryContainer,感觉几种 Container 颜色都怪怪的

如果 Card 用 surfaceContainer,Chip 用 sencondaryContainer 看上去还行,text 用 onSencondaryContainer

backgroundColor: Theme.of(context).colorScheme.secondaryContainer,
side: const BorderSide(style: BorderStyle.none),
image image

@garylkz
Copy link
Contributor Author

garylkz commented Feb 6, 2025

大致上已经修改完毕,如果没问题的话应该可以了

@Predidit
Copy link
Owner

Predidit commented Feb 6, 2025

感谢你的工作。我会在CI通过后合并。

@Predidit Predidit merged commit 7e56c9b into Predidit:main Feb 6, 2025
6 checks passed
ErBWs pushed a commit to ErBWs/Kazumi that referenced this pull request Feb 7, 2025
* Clip overflow sections for bangumi cards

* TextScale to prevent overflow instead

* Added back & simplified bangumi cards' padding

* Added text alignment to bangumi card

* Replaced `RichText` with `Text` for simplicity and readability

* Made BangumiHistoryCardV height scale by resolution

* Removed unused lines

* Use wrap chips instead of column texts to conserve space
- revert `mainAxisExtent` to fixed value to prevent overflow on low res

* Fixed wrap chips run spacing issue on mobile

* Calculate `mainAxisExtent` with font style
- reverted from using Wrap to Column for predictability
- implemented `Utils.getTextPxHeight`

* Changes again in favor of Wrap-Chip design
- simplified history page `mainAxisExtent` height formula
- merged `_PropertyText`, `_PropertyChip` into `BangumiHistoryCardV.propertyChip`

* Updated UI colors and parameter type handling, removed unused functions

(cherry picked from commit 7e56c9b)
@Predidit
Copy link
Owner

Predidit commented Feb 7, 2025

这是一个有必要但非常非常小的变更,我会直接提交它

@bggRGjQaUbCoE
Copy link
Contributor

bug
桌面端修改追番状态后,卡片背景固定为点击状态

@garylkz
Copy link
Contributor Author

garylkz commented Feb 7, 2025

bug
桌面端修改追番状态后,卡片背景固定为点击状态

好像是颜色改深了,点击卡片时颜色有改变吗?

@Predidit
Copy link
Owner

Predidit commented Feb 7, 2025

这个问题不是这个PR引入的,1.5.5 也有问题,需要调查一下

@Predidit
Copy link
Owner

Predidit commented Feb 7, 2025

问题可能来自 MenuAnchor 控件

@Predidit
Copy link
Owner

Predidit commented Feb 7, 2025

简单修了一下,总觉得这是 flutter 的 bug

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