Use correct channel icons in channel chat screen#444
Conversation
|
Just a note... Originally I only thought to fix the private channel icon, and that was a 2-line change. But as I dug in more, I finally understood what community channels are and how they work, and those icons were wrong too. Fixing that was much more involved, because it's not easy to tell whether a channel is part of a community. I ended up duplicating some things from channels_screen.dart. I tried a few times to refactor that to make it more reusable, but I didn't come up with anything I liked, and I decided this was better for now. |
zjs81
left a comment
There was a problem hiding this comment.
Thanks for the fix — the classification matches channels_screen.dart and renders correctly. A few things to address before merge:
- Dedupe the classification with
channels_screen.dart. The same 5-way logic now lives in two places (channels_screen._getCommunityForChannel/_isCommunityPublicChanneland the newgetChannelTypehere). Please pull this into a shared helper in this PR — e.g. a staticChannelType.classify(Channel channel, Iterable<Community> communities)next to the new enum inlib/models/channel.dart— and refactorchannels_screen.dartto use it too. The enum is already in the model, so the logic should live there as well; otherwise the two screens will drift. - Restore
size: 20on the AppBar icon — current code falls back to the default (24), which is a visible regression versus the previousIcon(..., size: 20). - Replace
default:with explicitcase ChannelType.private:so the Dart 3 switch is exhaustive and the analyzer warns when a new variant is added. - Prefix new helpers with
_(_getChannelType,_channelIcon) — they're state-internal and the rest of the file uses that convention. - First-frame flicker —
_loadCommunities()is fired async after the first frame, so community channels render asIcons.lock(private) until it resolves. Worth a placeholder or a synchronous cached read; see inline. - Minor: the
// Use a stack in case we want to add community annotationcomment is misleading — the Stack is already doing that.
No test changes needed.
| Future<void> _loadCommunities() async { | ||
| final connector = context.read<MeshCoreConnector>(); | ||
| _communityStore.setPublicKeyHex = connector.selfPublicKeyHex; | ||
| final communities = await _communityStore.loadCommunities(); |
There was a problem hiding this comment.
Fire-and-forget async load means a community channel renders as Icons.lock (the private default) on the first frame, then swaps once communities load. On slower devices this flickers visibly.
Two options:
- Render a neutral placeholder (or hide the icon) until
_communitieshas loaded at least once, or - Make
CommunityStore.loadCommunities()return a synchronously-cached list when one is available.
Also note: _communities never refreshes while this screen is open, so edits made elsewhere won't be reflected until the screen is recreated — acceptable for now but worth a one-line comment if you don't want to wire it to the store.
There was a problem hiding this comment.
Thanks. I'm new to Flutter & Dart, so I'm still getting up to speed on a lot of this.
Does channels_screen have the same flicker problem? It looks like the only difference between the two is that here _loadCommunities() is run from SchedulerBinding.instance.addPostFrameCallback and in channels_list it's from WidgetsBinding.
I regret to say I don't understand the second option.
Re the refresh note: I think I get it. We only load communities the first time, and if we navigate back here after changing them, the list will be stale? 20 minutes of reading about this stuff has mainly taught me that there are good idiomatic ways of managing this in Flutter, but I don't yet know what they are.
I force pushed what I have so far, which fixes all the issues you pointed out except this one.
There was a problem hiding this comment.
I'll do some testing I've noticed this before on my phone when I had lots of channels and I put them in custom positions.
| ChannelType getChannelType(Channel channel) { | ||
| // Go through the communities and their channels and see whether | ||
| // this channel matches one of them. | ||
| for (final community in _communities) { | ||
| final publicPsk = community.deriveCommunityPublicPsk(); | ||
| if (channel.pskHex == Channel.formatPskHex(publicPsk)) { | ||
| return ChannelType.communityPublic; | ||
| } | ||
| for (final hashtag in community.hashtagChannels) { | ||
| final hashtagPsk = community.deriveCommunityHashtagPsk(hashtag); | ||
| if (channel.pskHex == Channel.formatPskHex(hashtagPsk)) { | ||
| return ChannelType.communityHashtag; | ||
| } | ||
| } | ||
| } | ||
| if (channel.isPublicChannel) { | ||
| return ChannelType.public; | ||
| } else if (channel.name.startsWith('#')) { | ||
| return ChannelType.hashtag; | ||
| } | ||
| return ChannelType.private; | ||
| } |
There was a problem hiding this comment.
Please dedupe this with channels_screen.dart in this PR rather than as a follow-up.
channels_screen.dart:378-409 already classifies channels with the same logic via _getCommunityForChannel / _isCommunityPublicChannel. With the new ChannelType enum living in the model, the natural home for this is a static helper alongside it, e.g.:
// lib/models/channel.dart
enum ChannelType { public, private, hashtag, communityPublic, communityHashtag }
extension ChannelTypeClassify on Channel {
ChannelType classify(Iterable<Community> communities) {
for (final community in communities) {
if (pskHex == Channel.formatPskHex(community.deriveCommunityPublicPsk())) {
return ChannelType.communityPublic;
}
for (final hashtag in community.hashtagChannels) {
if (pskHex == Channel.formatPskHex(community.deriveCommunityHashtagPsk(hashtag))) {
return ChannelType.communityHashtag;
}
}
}
if (isPublicChannel) return ChannelType.public;
if (name.startsWith('#')) return ChannelType.hashtag;
return ChannelType.private;
}
}Then channel_chat_screen.dart calls widget.channel.classify(_communities) and channels_screen.dart is refactored to use the same call instead of its ad-hoc _getCommunityForChannel / _isCommunityPublicChannel. That keeps the two screens from drifting as new channel types are added.
Also rename to _getChannelType / drop entirely once this is extracted.
| children: [ | ||
| Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 3), | ||
| child: Icon(icon), |
There was a problem hiding this comment.
Visible regression: the previous AppBar icon was Icon(..., size: 20). This Icon(icon) falls back to the AppBar default (24), so the title-row icon is now noticeably larger than before.
| child: Icon(icon), | |
| child: Icon(icon, size: 20), |
If you keep the size, also scale the community badge Container (currently 14×14) proportionally so the annotation doesn't overflow the smaller icon.
| icon = Icons.lock; | ||
| } | ||
| return Stack( | ||
| // Use a stack in case we want to add community annotation |
There was a problem hiding this comment.
Nit: this comment is misleading — the Stack is already used for the community-badge annotation a few lines below, not "in case we want to add" one. Either remove or reword to describe what it actually does.
There was a problem hiding this comment.
I meant "in case we want to add one" because the inclusion of the badge is conditional, not for future development. But the comment clearly does more harm than good, so out it goes.
a1e8a94 to
0521df5
Compare
At the top of the channel chat screen is an icon, indicating the channel type. Previously, the public icon was used correctly, but the hashtag icon was used for all other types. Now, consistent with the channels screen, we use the lock icon for private channels, and the composite icons for community public & community hashtag types. The fix for private channels was trivial, as we can identify hashtag channels by their name. Finding out whether a channel belongs to a community is much more involved. All the hard-working code was copied from channels_screen.dart. (I tried refactoring to reduce duplication, but my results were complex and not worth it.) Closes zjs81#432
0521df5 to
2763d83
Compare
|
@zjs81 I didn't observe the flickering problem, so I'm taking my fix on faith. Probably I should get set up with some virtual test devices. |
|
Thanks I'll review it again. |
At the top of the channel chat screen is an icon, indicating the channel type.
Previously, the public icon was used correctly, but the hashtag icon was used for all other types.
Now, consistent with the channels screen, we use the lock icon for private channels, and the composite icons for community public & community hashtag types.
Closes issue #432.