UnigramDev/Unigram/develop • e11f632 • 3 files, +244/-90
Fan out the batched TDLib requests, and triage the rest of P2
Applying the rate check that reversed the ConcurrentDictionary item to the rest
of the performance section. Three of the eight items survive it, two are fixed
elsewhere in kind, and two are closed as not worth doing.
The one that gets stronger under scrutiny is the batched requests, because it is
latency rather than throughput and it sits on a path a person waits on.
GetMessageProperties was issued one message at a time, so selecting a hundred
messages meant a hundred sequential round trips before the toolbar could decide
which actions to offer. That and the three others — reactions, custom emoji
sticker sets, message effects — now issue their requests together and await them
as a group.
Two things worth knowing about that change. GetMessageEffectsAsync keeps its
results in request order by indexing them by position: the effect drawer and the
reaction menu both display them in the order they asked for, and the obvious
rewrite of appending fetched results after cached ones silently reorders them.
And the caches are still written in one loop after the group completes, on one
thread, so this does not worsen the open finding about _cachedReactions being an
unsynchronised Dictionary. GetAllReactionsAsync was a verbatim copy of
GetReactionsAsync and now calls it.
OwnedStarCount and OwnedGramCount sent a request on every read until the update
landed, and they are read from bindings, which re-evaluate. Guarded now, and
reset in Clear() so a new authorization fetches again.
GetChatFolders allocated a closure over this on every chat cell that showed a
folder tag. That is now a field built once. The O(n log n) framing in the review
was overstated: a chat is usually in one or two folders, so the sort ran about
one comparison, and a chat in none allocates nothing at all. Rewriting it to
walk the folder list instead would have been slower, since it turns the common
empty case from scanning two entries into scanning every folder.
ReaderWriterDictionary.Find wrapped its predicate in a lambda for
FirstOrDefault, allocating a closure and an enumerator per call.
Closed without changes, both recorded in the review with the arithmetic: the
105-case type switch in OnResult costs 100-200ns against an update rate of tens
per second normally and thousands during a sync, so a dispatch table buys
nothing; and the service construction inside GetChats cannot move to
UpdateSupergroup, because that update carries a supergroup id and there is no
supergroup-to-chat index to get back to the Chat, making GetChats the only place
that notices a supergroup which became a forum after its updateNewChat.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 9b56b0c • 1 files, +44/-23
Record the review decisions, and what m_impl is actually racing
Five open questions were accepted as they stand and one deferred upstream, so
the list of genuinely open items is down to three.
The m_impl item was filed as theoretical and is not. VoipManager is safe, since
every managed call site including Dispose runs under _managerLock. VoipGroupCall
has no such lock and reaches Dispose from TDLib update thread while the UI thread
calls in, so Stop resetting m_impl there is a use-after-free.
Also corrects the reason I gave for leaving it alone: guarding m_impl would not
put a lock back on the path into managed code, because the callbacks never touch
m_impl.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Fan out the batched TDLib requests, and triage the rest of P2
Applying the rate check that reversed the ConcurrentDictionary item to the rest
of the performance section. Three of the eight items survive it, two are fixed
elsewhere in kind, and two are closed as not worth doing.
The one that gets stronger under scrutiny is the batched requests, because it is
latency rather than throughput and it sits on a path a person waits on.
GetMessageProperties was issued one message at a time, so selecting a hundred
messages meant a hundred sequential round trips before the toolbar could decide
which actions to offer. That and the three others — reactions, custom emoji
sticker sets, message effects — now issue their requests together and await them
as a group.
Two things worth knowing about that change. GetMessageEffectsAsync keeps its
results in request order by indexing them by position: the effect drawer and the
reaction menu both display them in the order they asked for, and the obvious
rewrite of appending fetched results after cached ones silently reorders them.
And the caches are still written in one loop after the group completes, on one
thread, so this does not worsen the open finding about _cachedReactions being an
unsynchronised Dictionary. GetAllReactionsAsync was a verbatim copy of
GetReactionsAsync and now calls it.
OwnedStarCount and OwnedGramCount sent a request on every read until the update
landed, and they are read from bindings, which re-evaluate. Guarded now, and
reset in Clear() so a new authorization fetches again.
GetChatFolders allocated a closure over this on every chat cell that showed a
folder tag. That is now a field built once. The O(n log n) framing in the review
was overstated: a chat is usually in one or two folders, so the sort ran about
one comparison, and a chat in none allocates nothing at all. Rewriting it to
walk the folder list instead would have been slower, since it turns the common
empty case from scanning two entries into scanning every folder.
ReaderWriterDictionary.Find wrapped its predicate in a lambda for
FirstOrDefault, allocating a closure and an enumerator per call.
Closed without changes, both recorded in the review with the arithmetic: the
105-case type switch in OnResult costs 100-200ns against an update rate of tens
per second normally and thousands during a sync, so a dispatch table buys
nothing; and the service construction inside GetChats cannot move to
UpdateSupergroup, because that update carries a supergroup id and there is no
supergroup-to-chat index to get back to the Chat, making GetChats the only place
that notices a supergroup which became a forum after its updateNewChat.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 9b56b0c • 1 files, +44/-23
Record the review decisions, and what m_impl is actually racing
Five open questions were accepted as they stand and one deferred upstream, so
the list of genuinely open items is down to three.
The m_impl item was filed as theoretical and is not. VoipManager is safe, since
every managed call site including Dispose runs under _managerLock. VoipGroupCall
has no such lock and reaches Dispose from TDLib update thread while the UI thread
calls in, so Stop resetting m_impl there is a use-after-free.
Also corrects the reason I gave for leaving it alone: guarding m_impl would not
put a lock back on the path into managed code, because the callbacks never touch
m_impl.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/develop • a0b0763 • 1 files, +37/-10
Close the unbounded growth item: _files is TDLib's model
Fela's read is right, and the reason is sharper than "a TDLib issue". The
contract is id-to-instance identity: updateFile carries a file id and ParseFile
mutates the existing instance in place, so every binding already holding that
File sees the change. TDLib never retires a file id within a session and never
signals that one is finished with. Drop an entry and the next update for that id
mints a new instance while the UI holds the old one, and that thumbnail stops
updating for good. Eviction is only safe when nothing holds the entry, which
means weak references plus a sweep plus a dereference per update on the receive
thread — a lot of machinery for the size involved.
Which the item never stated. One entry is three objects and three strings, the
strings dominating at roughly 700-1000 bytes, and every photo contributes an id
per size variant. Ten thousand files is about 8MB and a hundred thousand about
80MB: real, but not the multi-GB growth being chased elsewhere.
The three download sets were in this item only because they sit next to each
other in the file. They are ours rather than TDLib's, and they are hundreds of
KB at the top end. All four are already dropped on an authorization change by
the earlier Clear() fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 404313f • 2 files, +50/-12
Retry a forum topic that failed to load for a transient reason
UpdateNewTopic returned on any non-ForumTopic response before removing the id
from _pendingNewTopics, and GetTopic only requests a topic it has not already
asked for. So one failed load hid a topic for the rest of the session: the
method returned null forever and nothing ever asked again.
Clearing the entry on every failure would have been the worse bug. The
suppression is load-bearing — a topic that genuinely does not exist would
otherwise be requested again on every enumeration of the list, one round trip
per scroll, forever. It is also the only thing currently keeping the bogus
int.MaxValue lookup in GetTopics from repeating.
So the retry is scoped to failures that repeating can fix: code 500 and above,
or below zero, meaning server or transport. Every 4xx stays suppressed, since it
says the request is wrong or the topic is gone. Keying on 404 alone would not
have been enough, because TDLib reports a missing object as 400 at least as
often, which would have left the storm open through the more common code.
UpdateNewTopic now takes the id it asked for, a failure response carrying none
of its own. The call site inside UpdateDeleteMessages names its lambda parameter
inner, the enclosing callback having already bound response.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Close the unbounded growth item: _files is TDLib's model
Fela's read is right, and the reason is sharper than "a TDLib issue". The
contract is id-to-instance identity: updateFile carries a file id and ParseFile
mutates the existing instance in place, so every binding already holding that
File sees the change. TDLib never retires a file id within a session and never
signals that one is finished with. Drop an entry and the next update for that id
mints a new instance while the UI holds the old one, and that thumbnail stops
updating for good. Eviction is only safe when nothing holds the entry, which
means weak references plus a sweep plus a dereference per update on the receive
thread — a lot of machinery for the size involved.
Which the item never stated. One entry is three objects and three strings, the
strings dominating at roughly 700-1000 bytes, and every photo contributes an id
per size variant. Ten thousand files is about 8MB and a hundred thousand about
80MB: real, but not the multi-GB growth being chased elsewhere.
The three download sets were in this item only because they sit next to each
other in the file. They are ours rather than TDLib's, and they are hundreds of
KB at the top end. All four are already dropped on an authorization change by
the earlier Clear() fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 404313f • 2 files, +50/-12
Retry a forum topic that failed to load for a transient reason
UpdateNewTopic returned on any non-ForumTopic response before removing the id
from _pendingNewTopics, and GetTopic only requests a topic it has not already
asked for. So one failed load hid a topic for the rest of the session: the
method returned null forever and nothing ever asked again.
Clearing the entry on every failure would have been the worse bug. The
suppression is load-bearing — a topic that genuinely does not exist would
otherwise be requested again on every enumeration of the list, one round trip
per scroll, forever. It is also the only thing currently keeping the bogus
int.MaxValue lookup in GetTopics from repeating.
So the retry is scoped to failures that repeating can fix: code 500 and above,
or below zero, meaning server or transport. Every 4xx stays suppressed, since it
says the request is wrong or the topic is gone. Keying on 404 alone would not
have been enough, because TDLib reports a missing object as 400 at least as
often, which would have left the storm open through the more common code.
UpdateNewTopic now takes the id it asked for, a failure response carrying none
of its own. The call site inside UpdateDeleteMessages names its lambda parameter
inner, the enclosing callback having already bound response.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/develop • 2925804 • 2 files, +28/-14
Pass AUTOCONVERTPCM as a stream flag, not as the periodicity
Initialize takes it in StreamFlags; it sat in hnsPeriodicity, which shared mode
requires to be 0. Inherited from the Microsoft sample.
Measured rather than assumed, with a harness that activated the same process
loopback client three ways: as shipped, corrected, and with the flag dropped
entirely. All three return S_OK with the same 480 frame buffer, so shared mode
ignores periodicity instead of validating it and screen audio was never at risk.
The control run also shows the flag itself does nothing here, since process
loopback converts to whatever format is asked for.
A tidy-up then, not a fix. The measurement is in the comment so the next reader
does not have to repeat it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 73e57c5 • 2 files, +36/-9
Refresh every topic a delete touched, not just the first
The break sat inside the loop over the deleted ids, so only the first topic
whose last message had gone got asked for a new one. Clearing a chat's history,
or deleting everything one member ever sent, takes out the last message of many
topics at once, and all but one were left showing a preview of a message that no
longer exists — and sorted by it, so they held their old position in the list
too.
Each _messages entry is still handled at most once: the entry is removed as it is
handled, so a later id in the batch resolves to a different one. That is per
entry rather than per topic, because LoadForumTopicsAsync can leave a stale entry
behind for a topic it reloads — noted in the review as its own item, since it
means the one-entry-per-topic property cannot be leaned on. Hitting a stale entry
costs a redundant refresh, never a wrong one.
The cost of the fix is that a delete taking out the last message of n topics now
issues n getForumTopic calls where it issued one. That is bounded by the topics
actually affected, and the alternative of reloading the whole list is a much
larger change to the batch-load path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • a643a23 • 4 files, +12/-3014
Fix record button on Windows 10
UnigramDev/Unigram/develop • 025878b • 1 files, +5/-0
DIsable quote for rich messages
UnigramDev/Unigram/develop • 5942885 • 3 files, +258/-0
Add claude hooks
#unigram
Pass AUTOCONVERTPCM as a stream flag, not as the periodicity
Initialize takes it in StreamFlags; it sat in hnsPeriodicity, which shared mode
requires to be 0. Inherited from the Microsoft sample.
Measured rather than assumed, with a harness that activated the same process
loopback client three ways: as shipped, corrected, and with the flag dropped
entirely. All three return S_OK with the same 480 frame buffer, so shared mode
ignores periodicity instead of validating it and screen audio was never at risk.
The control run also shows the flag itself does nothing here, since process
loopback converts to whatever format is asked for.
A tidy-up then, not a fix. The measurement is in the comment so the next reader
does not have to repeat it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 73e57c5 • 2 files, +36/-9
Refresh every topic a delete touched, not just the first
The break sat inside the loop over the deleted ids, so only the first topic
whose last message had gone got asked for a new one. Clearing a chat's history,
or deleting everything one member ever sent, takes out the last message of many
topics at once, and all but one were left showing a preview of a message that no
longer exists — and sorted by it, so they held their old position in the list
too.
Each _messages entry is still handled at most once: the entry is removed as it is
handled, so a later id in the batch resolves to a different one. That is per
entry rather than per topic, because LoadForumTopicsAsync can leave a stale entry
behind for a topic it reloads — noted in the review as its own item, since it
means the one-entry-per-topic property cannot be leaned on. Hitting a stale entry
costs a redundant refresh, never a wrong one.
The cost of the fix is that a delete taking out the last message of n topics now
issues n getForumTopic calls where it issued one. That is bounded by the topics
actually affected, and the alternative of reloading the whole list is a much
larger change to the batch-load path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • a643a23 • 4 files, +12/-3014
Fix record button on Windows 10
UnigramDev/Unigram/develop • 025878b • 1 files, +5/-0
DIsable quote for rich messages
UnigramDev/Unigram/develop • 5942885 • 3 files, +258/-0
Add claude hooks
#unigram
UnigramDev/Unigram/develop • c715efe • 3 files, +30/-8
Stop asking the server for the synthetic topic row
Both GetTopics implementations yield a synthetic row for a sentinel id — the
"All topics" entry, or the new-topic prompt in a bot chat — and then fall
through to GetTopic with that same sentinel. In the forum service that fires
getForumTopic for id 2147483647, and the failure then sits in _pendingNewTopics
for the life of the service, since a 4xx is deliberately not retried. One wasted
round trip per forum opened, and a permanently poisoned entry.
A continue in each. The direct-messages one was harmless today only because its
GetTopic is cache-only rather than fetching, which is not a property worth
relying on.
Left alone deliberately: the four allocations per enumeration for that synthetic
row. The review called it a constant to hoist into a field, and it is not one.
Its label comes from Strings.AllTopicsShort, which is a live Resource.GetString
call, and the app applies updateLanguagePackStrings at runtime while a
ForumTopicService lives until logout. A hoisted field would keep showing the
previous language for the rest of the session. Four allocations on a list
enumeration is not worth a visibly wrong string.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 90c8623 • 1 files, +23/-16
Downgrade GetChatFromMessageSenderAsync: it is not a bug
The review claimed the method returns null for every chat sender as though the
chat were meant to be returned. Read on its own it does one coherent thing:
resolve a user sender to its private chat, creating it if needed, and return null
for anything else. For a MessageSenderChat both paths return null, found or not
found, so there is no inconsistency and nothing is dropped.
What is really there is a vestigial first line. TryGetChat(messageSender, out
chat) has its return value discarded and its out value can only be non-null in
exactly the case the following if excludes, so it is dead on every path — and it
is what makes the method read as though chat senders were handled.
Kept in the doc at P3 with a note on how the wrong conclusion was reached: the
reasoning followed the cached branch, saw the value discarded, and stopped,
without checking that the uncached branch returns null too — which is what shows
the behaviour is uniform and deliberate. Reading the call sites afterwards made
it look confirmed, since a channel receiver really does get null; that just is
not a defect, because the purchase uses the sender directly and succeeds, and the
chat only picks which toast is shown.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 6de9291 • 3 files, +30/-47
Fix leak instrumentation
#unigram
Stop asking the server for the synthetic topic row
Both GetTopics implementations yield a synthetic row for a sentinel id — the
"All topics" entry, or the new-topic prompt in a bot chat — and then fall
through to GetTopic with that same sentinel. In the forum service that fires
getForumTopic for id 2147483647, and the failure then sits in _pendingNewTopics
for the life of the service, since a 4xx is deliberately not retried. One wasted
round trip per forum opened, and a permanently poisoned entry.
A continue in each. The direct-messages one was harmless today only because its
GetTopic is cache-only rather than fetching, which is not a property worth
relying on.
Left alone deliberately: the four allocations per enumeration for that synthetic
row. The review called it a constant to hoist into a field, and it is not one.
Its label comes from Strings.AllTopicsShort, which is a live Resource.GetString
call, and the app applies updateLanguagePackStrings at runtime while a
ForumTopicService lives until logout. A hoisted field would keep showing the
previous language for the rest of the session. Four allocations on a list
enumeration is not worth a visibly wrong string.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 90c8623 • 1 files, +23/-16
Downgrade GetChatFromMessageSenderAsync: it is not a bug
The review claimed the method returns null for every chat sender as though the
chat were meant to be returned. Read on its own it does one coherent thing:
resolve a user sender to its private chat, creating it if needed, and return null
for anything else. For a MessageSenderChat both paths return null, found or not
found, so there is no inconsistency and nothing is dropped.
What is really there is a vestigial first line. TryGetChat(messageSender, out
chat) has its return value discarded and its out value can only be non-null in
exactly the case the following if excludes, so it is dead on every path — and it
is what makes the method read as though chat senders were handled.
Kept in the doc at P3 with a note on how the wrong conclusion was reached: the
reasoning followed the cached branch, saw the value discarded, and stopped,
without checking that the uncached branch returns null too — which is what shows
the behaviour is uniform and deliberate. Reading the call sites afterwards made
it look confirmed, since a channel receiver really does get null; that just is
not a defect, because the purchase uses the sender directly and succeeds, and the
chat only picks which toast is shown.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 6de9291 • 3 files, +30/-47
Fix leak instrumentation
#unigram
UnigramDev/Unigram/tdjson-symbols • 079689f • 1 files, +4/-0
Ship tdjson symbols in the appxsym
tdjson.dll is added straight to ReferenceCopyLocalPaths, which is an output
of reference resolution rather than an input, so ResolveAssemblyReference
never sees it and never runs the related-file discovery that picks up a .pdb
sitting beside a resolved reference. RLottie gets its symbols for free that
way; tdjson has to name the file.
The appxsym is built from AppxPackagePayload filtered to .pdb, and payload
includes copy-local items, so naming it here is enough. PDBs are removed
from the .appx itself afterwards, so this does not grow the installed app.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Ship tdjson symbols in the appxsym
tdjson.dll is added straight to ReferenceCopyLocalPaths, which is an output
of reference resolution rather than an input, so ResolveAssemblyReference
never sees it and never runs the related-file discovery that picks up a .pdb
sitting beside a resolved reference. RLottie gets its symbols for free that
way; tdjson has to name the file.
The appxsym is built from AppxPackagePayload filtered to .pdb, and payload
includes copy-local items, so naming it here is enough. PDBs are removed
from the .appx itself afterwards, so this does not grow the installed app.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
telegramdesktop/tdesktop/nightly • 314d2f1 • 1 files, +17/-12
Switched nightly Windows CI with optimized binaries without arm64.
#tdesktop
Switched nightly Windows CI with optimized binaries without arm64.
#tdesktop
🫡3
telegramdesktop/tdesktop/nightly • 4da7854 • 1 files, +17/-12
Switched nightly Windows CI with optimized binaries without arm64.
#tdesktop
Switched nightly Windows CI with optimized binaries without arm64.
#tdesktop
🫡2
UnigramDev/Unigram/chatlist-template-init • 03bdd04 • 2 files, +45/-16
Fix NullReferenceException when the chat list loads before its template
OnLoaded dereferenced ScrollViewer.ContentTemplateRoot, but ScrollViewer is
only assigned in OnApplyTemplate, which runs on the first measure pass. A
control that is in the tree and never measured raises Loaded with the template
parts still null, and the handler threw.
The element it wants is the ItemsPresenter the template already declares, so
name it and read it with GetTemplateChild like the other parts. That drops the
dependency on the ScrollViewer's ContentPresenter having realized, and the
setup can then run from whichever of OnApplyTemplate and Loaded arrives second.
Returning early instead would have left _trackerOwner null for the session and
the swipe-between-folders carousel silently dead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Fix NullReferenceException when the chat list loads before its template
OnLoaded dereferenced ScrollViewer.ContentTemplateRoot, but ScrollViewer is
only assigned in OnApplyTemplate, which runs on the first measure pass. A
control that is in the tree and never measured raises Loaded with the template
parts still null, and the handler threw.
The element it wants is the ItemsPresenter the template already declares, so
name it and read it with GetTemplateChild like the other parts. That drops the
dependency on the ScrollViewer's ContentPresenter having realized, and the
setup can then run from whichever of OnApplyTemplate and Loaded arrives second.
Returning early instead would have left _trackerOwner null for the session and
the swipe-between-folders carousel silently dead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
telegramdesktop/tdesktop/nightly • ee68701 • 3 files, +23/-28
Switched nightly Windows CI with optimized binaries without arm64.
#tdesktop
Switched nightly Windows CI with optimized binaries without arm64.
#tdesktop
🫡2
UnigramDev/Unigram/exception-reporting • 599c5e7 • 2 files, +55/-14
Keep the originating description when an unhandled error is a bare E_FAIL
A large share of unhandled errors reach OnUnhandledExceptionDetected as an
E_FAIL with no message and a stack that only shows Propagate() rethrowing it.
GetStowedException already tries to recover the real context, but it returns
null unless every step succeeds, so those reports carry nothing.
Read the description out of IRestrictedErrorInfo before giving up, and return
what was recovered even when the stowed frames are unavailable. The call was
already written out in a comment and left unused on the assumption that the
propagated managed exception would carry the details - which is exactly what
fails here.
WatchDog no longer overwrites the recovered string with the empty one from the
propagated exception.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Keep the originating description when an unhandled error is a bare E_FAIL
A large share of unhandled errors reach OnUnhandledExceptionDetected as an
E_FAIL with no message and a stack that only shows Propagate() rethrowing it.
GetStowedException already tries to recover the real context, but it returns
null unless every step succeeds, so those reports carry nothing.
Read the description out of IRestrictedErrorInfo before giving up, and return
what was recovered even when the stowed frames are unavailable. The call was
already written out in a comment and left unused on the assumption that the
propagated managed exception would carry the details - which is exactly what
fails here.
WatchDog no longer overwrites the recovered string with the empty one from the
propagated exception.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • d7b1449 • 1 files, +31/-8
Reconcile the review checkboxes with what actually landed
The E2E delegate signature item was still marked open although the byte[] work
closed it, and the transform semaphore it exposed was not written down at all.
The five questions Fela ruled on are marked decided rather than outstanding, and
mono screencast audio as deferred upstream.
Two genuinely open items remain: m_impl unguarded in VoipGroupManager, and the
unmeasured buffer between the WASAPI and WebRTC clocks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • e8aa325 • 1 files, +41/-8
Re-aim the NewDictionary finding at Clear()
The item claimed the mutating indexer getter, plus _haveFullChatList being set
outside the monitor, amounted to a cross-thread race. It does not. All seven
callers of GetChatListAsync and GetStoryListAsync are UI-layer and the repo has
no ConfigureAwait(false), so those continuations resume on the UI thread, and
every use of the mutating getter is already inside a lock. No second thread reads
those dictionaries under the monitor at all. Demoted to P3 as a trap for the next
reader rather than a defect.
The real cross-thread party is Clear(), which runs on the TDLib receive thread
and takes two locks while emptying eight collections that every other accessor
guards — _chatList and _haveFullChatList, _storyList and _haveFullStoryList,
_savedMessages, _savedMessagesTags, _suggestedActions and _chatFolders2. A logout
arriving while the UI has a chat list load in flight can clear a SortedSet out
from under an enumerator.
Which the earlier Clear() fix missed: it audited which fields were cleared and
never asked whether clearing them was synchronised.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 1632e05 • 2 files, +47/-14
Hold the locks while Clear() empties the collections they guard
Clear() runs on the TDLib receive thread, from OnResult on
AuthorizationStateClosed, and was taking two locks while emptying nine
collections that every other accessor guards. A logout landing while the UI had a
chat list load in flight could clear a SortedSet out from under an enumerator, or
a Dictionary mid-lookup.
Each group now takes the same lock its readers do: _chatList with
_haveFullChatList, _storyList with _haveFullStoryList, _savedMessages,
_savedMessagesTags, _suggestedActions, _chatFolders with _chatFolders2, and
_timezones. They are taken one after another and never nested, so no lock
ordering is introduced.
_timezones was not in the list this started from. Enumerating every lock target
across the partials found it, which is the check worth repeating when a field is
added — the same lesson as the coverage audit, one level down.
This is what the earlier Clear() fix missed: it audited which fields were
cleared, and never asked whether clearing them was synchronised.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Reconcile the review checkboxes with what actually landed
The E2E delegate signature item was still marked open although the byte[] work
closed it, and the transform semaphore it exposed was not written down at all.
The five questions Fela ruled on are marked decided rather than outstanding, and
mono screencast audio as deferred upstream.
Two genuinely open items remain: m_impl unguarded in VoipGroupManager, and the
unmeasured buffer between the WASAPI and WebRTC clocks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • e8aa325 • 1 files, +41/-8
Re-aim the NewDictionary finding at Clear()
The item claimed the mutating indexer getter, plus _haveFullChatList being set
outside the monitor, amounted to a cross-thread race. It does not. All seven
callers of GetChatListAsync and GetStoryListAsync are UI-layer and the repo has
no ConfigureAwait(false), so those continuations resume on the UI thread, and
every use of the mutating getter is already inside a lock. No second thread reads
those dictionaries under the monitor at all. Demoted to P3 as a trap for the next
reader rather than a defect.
The real cross-thread party is Clear(), which runs on the TDLib receive thread
and takes two locks while emptying eight collections that every other accessor
guards — _chatList and _haveFullChatList, _storyList and _haveFullStoryList,
_savedMessages, _savedMessagesTags, _suggestedActions and _chatFolders2. A logout
arriving while the UI has a chat list load in flight can clear a SortedSet out
from under an enumerator.
Which the earlier Clear() fix missed: it audited which fields were cleared and
never asked whether clearing them was synchronised.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 1632e05 • 2 files, +47/-14
Hold the locks while Clear() empties the collections they guard
Clear() runs on the TDLib receive thread, from OnResult on
AuthorizationStateClosed, and was taking two locks while emptying nine
collections that every other accessor guards. A logout landing while the UI had a
chat list load in flight could clear a SortedSet out from under an enumerator, or
a Dictionary mid-lookup.
Each group now takes the same lock its readers do: _chatList with
_haveFullChatList, _storyList with _haveFullStoryList, _savedMessages,
_savedMessagesTags, _suggestedActions, _chatFolders with _chatFolders2, and
_timezones. They are taken one after another and never nested, so no lock
ordering is introduced.
_timezones was not in the list this started from. Enumerating every lock target
across the partials found it, which is the check worth repeating when a field is
added — the same lesson as the coverage audit, one level down.
This is what the earlier Clear() fix missed: it audited which fields were
cleared, and never asked whether clearing them was synchronised.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • bdf4304 • 3 files, +50/-13
Make the three loose dictionaries thread-safe, mostly by type
_chatAccessibleUntil, _cachedReactions and _preparedLogsFileIds were plain
collections shared between the UI thread and the TDLib one. The first two are
written and read from the UI and emptied by Clear() on the receive thread; the
third is worse, and not merely latent: PrepareLogs does ??= new() then Add() on
the UI thread while UpdateFile assigns the field null on the TDLib thread, so a
call landing between those two statements dereferences null.
Fela's point on approach is right, so the rule is now explicit rather than picked
per site. A standalone dictionary whose operations are single calls becomes a
ReaderWriterDictionary, like the rest of the caches. A lock is only for what that
type cannot express: a set rather than a dictionary, a field assigned null, or an
operation that has to be compound.
So _chatAccessibleUntil and _cachedReactions are ReaderWriterDictionary now and
carry no explicit locking at all — the call sites got shorter rather than longer.
_preparedLogsFileIds fails all three tests, so it keeps a lock and says why where
it is declared. Its remove-then-maybe-reset is one critical section now, with the
Client.Execute that restores the verbosity hoisted out of it.
That same rule is what put ForumTopicService on a single lock earlier: six of its
eight collections are sets, a list and a sorted set, with compounds spanning
them, so the type would have covered two of eight and left two lock domains.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 43afd24 • 1 files, +18/-2
Close D3: the _haveFullList write races with nothing
The item said it was the same shape as the _haveFullChatList finding above it,
and it was — including being wrong for the same reason, so it inherits the
correction rather than the bug.
_haveFullList is a plain bool with three references: the declaration, one read
inside the lock and one write outside it. Its only caller is TopicListViewModel,
and with no ConfigureAwait(false) anywhere in the repo that continuation resumes
on the UI thread, the same one that took the lock. The TDLib thread reaches this
class only through UpdateDirectMessagesChatTopic, which never touches the field.
Granting a second thread anyway, a bool write is atomic, so the worst case is one
redundant loadDirectMessagesChatTopics rather than corruption.
The same reasoning clears ForumTopicService._haveFullList, which has the
identical shape and was never raised. In both, the await completes on a
TaskCompletionSource set from the TDLib thread, but the continuation posts back
to the UI context — which is what makes the lock-then-unlocked-write sequence
single-threaded in the first place.
No code change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Make the three loose dictionaries thread-safe, mostly by type
_chatAccessibleUntil, _cachedReactions and _preparedLogsFileIds were plain
collections shared between the UI thread and the TDLib one. The first two are
written and read from the UI and emptied by Clear() on the receive thread; the
third is worse, and not merely latent: PrepareLogs does ??= new() then Add() on
the UI thread while UpdateFile assigns the field null on the TDLib thread, so a
call landing between those two statements dereferences null.
Fela's point on approach is right, so the rule is now explicit rather than picked
per site. A standalone dictionary whose operations are single calls becomes a
ReaderWriterDictionary, like the rest of the caches. A lock is only for what that
type cannot express: a set rather than a dictionary, a field assigned null, or an
operation that has to be compound.
So _chatAccessibleUntil and _cachedReactions are ReaderWriterDictionary now and
carry no explicit locking at all — the call sites got shorter rather than longer.
_preparedLogsFileIds fails all three tests, so it keeps a lock and says why where
it is declared. Its remove-then-maybe-reset is one critical section now, with the
Client.Execute that restores the verbosity hoisted out of it.
That same rule is what put ForumTopicService on a single lock earlier: six of its
eight collections are sets, a list and a sorted set, with compounds spanning
them, so the type would have covered two of eight and left two lock domains.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 43afd24 • 1 files, +18/-2
Close D3: the _haveFullList write races with nothing
The item said it was the same shape as the _haveFullChatList finding above it,
and it was — including being wrong for the same reason, so it inherits the
correction rather than the bug.
_haveFullList is a plain bool with three references: the declaration, one read
inside the lock and one write outside it. Its only caller is TopicListViewModel,
and with no ConfigureAwait(false) anywhere in the repo that continuation resumes
on the UI thread, the same one that took the lock. The TDLib thread reaches this
class only through UpdateDirectMessagesChatTopic, which never touches the field.
Granting a second thread anyway, a bool write is atomic, so the worst case is one
redundant loadDirectMessagesChatTopics rather than corruption.
The same reasoning clears ForumTopicService._haveFullList, which has the
identical shape and was never raised. In both, the await completes on a
TaskCompletionSource set from the TDLib thread, but the continuation posts back
to the UI context — which is what makes the lock-then-unlocked-write sequence
single-threaded in the first place.
No code change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • 806ba3d • 2 files, +299/-192
Guard the group call managers against Dispose
VoipGroupCall reaches Dispose from Update on TDLib update thread while the UI
thread calls SetVolume, AddIncomingVideoOutput and the rest, and stopping a
manager frees the tgcalls instance the other thread may be part way into. Its
sibling VoipCall has always serialised this behind _managerLock; VoipGroupCall
declared the same field but only ever wrapped a block of commented out code
copied along with it.
Held now around every call that reaches the instance, and around the teardown in
Dispose and EndScreenSharing. Left outside: the IsMuted and
IsNoiseSuppressionEnabled getters, which read a native field rather than
touching m_impl, and the null checks that only decide whether to start
something.
Two spots needed more than a wrapper. The EmitJoinPayload continuations run
after awaits, so they take the lock again where they call in, since C# cannot
hold one across an await. And UpdateParticipant dereferenced _manager
unconditionally from the update thread, which was a null reference waiting to
happen quite apart from the race.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • a94e531 • 2 files, +16/-37
Flatten buttons text
UnigramDev/Unigram/composer-typing-performance • 4731dca • 2 files, +34/-5
Check a file against the disk only the first time it is seen
ParseFile called NativeUtils.FileExists after every parse, for files it had seen
thousands of times as readily as for new ones. Every parsed object carries files
— a history page is hundreds of them, nearly all already cached — and this runs
on the thread draining td_receive, so each one spent a syscall re-answering a
question already answered.
ProcessFile, on the old type-crossed parser, only ever checked a file id it had
not seen. That is the right shape and ParseFile now matches it, which drops the
steady-state cost from per-file-per-update to once per file id per session.
Nothing real is lost by not repeating it. TDLib sends no update when a file
disappears behind its back, so the repeat check only caught an external delete
when some unrelated update happened to arrive for that same file — luck rather
than detection. The reliable path is GetFileAsync catching FileNotFoundException
where the file is actually used. What the first-sight check is genuinely for is
the cache having been cleared between sessions, and that still works.
This does not help the startup replay, where every file is a first sight by
definition. Whether that burst is worth deferring off the receive thread is
worth counting before building anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Guard the group call managers against Dispose
VoipGroupCall reaches Dispose from Update on TDLib update thread while the UI
thread calls SetVolume, AddIncomingVideoOutput and the rest, and stopping a
manager frees the tgcalls instance the other thread may be part way into. Its
sibling VoipCall has always serialised this behind _managerLock; VoipGroupCall
declared the same field but only ever wrapped a block of commented out code
copied along with it.
Held now around every call that reaches the instance, and around the teardown in
Dispose and EndScreenSharing. Left outside: the IsMuted and
IsNoiseSuppressionEnabled getters, which read a native field rather than
touching m_impl, and the null checks that only decide whether to start
something.
Two spots needed more than a wrapper. The EmitJoinPayload continuations run
after awaits, so they take the lock again where they call in, since C# cannot
hold one across an await. And UpdateParticipant dereferenced _manager
unconditionally from the update thread, which was a null reference waiting to
happen quite apart from the race.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • a94e531 • 2 files, +16/-37
Flatten buttons text
UnigramDev/Unigram/composer-typing-performance • 4731dca • 2 files, +34/-5
Check a file against the disk only the first time it is seen
ParseFile called NativeUtils.FileExists after every parse, for files it had seen
thousands of times as readily as for new ones. Every parsed object carries files
— a history page is hundreds of them, nearly all already cached — and this runs
on the thread draining td_receive, so each one spent a syscall re-answering a
question already answered.
ProcessFile, on the old type-crossed parser, only ever checked a file id it had
not seen. That is the right shape and ParseFile now matches it, which drops the
steady-state cost from per-file-per-update to once per file id per session.
Nothing real is lost by not repeating it. TDLib sends no update when a file
disappears behind its back, so the repeat check only caught an external delete
when some unrelated update happened to arrive for that same file — luck rather
than detection. The reliable path is GetFileAsync catching FileNotFoundException
where the file is actually used. What the first-sight check is genuinely for is
the cache having been cleared between sessions, and that still works.
This does not help the startup replay, where every file is a first sight by
definition. Whether that burst is worth deferring off the receive thread is
worth counting before building anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • a6396fa • 6 files, +325/-79
Move the SendFilesPopup thumbnails off the models
StorageMedia._preview was assigned in four places and released in none, and
the models outlive the popup — SendFileExecute hands them to the send loop —
so every decoded thumbnail stayed alive for the whole send. A photo costs
roughly a megabyte and a video twice that, so a large drop retained tens of
megabytes with nothing left to free them.
Preview/Refresh/RefreshAsync are gone. The decode moves verbatim into
StorageThumbnailCache, owned by the popup, which pushes into the ImageBrush
instead of being pulled through a binding. Entries are dropped as their album
container is recycled, so the live set is now bounded by what the ListView has
realized rather than by how many files were picked, and cleared outright on
unload.
The cache also coalesces: Refresh() was async void fired from the Preview
getter with no in-flight guard, and _preview stayed null across the await, so
every container that asked before the first decode returned started another
decode of the same file — and the album panel rebuilds all of its children on
every UpdatePanel.
A decode that finishes after its entry was released, invalidated or cleared is
dropped rather than cached. That is what stops a post-recycle result from
resurrecting a bitmap nothing is left to release, and a pre-crop image from
landing after Invalidate.
The StorageVideo.Refresh override goes with it. It re-ran LoadPreview() after
a crop, which only feeds MaxCompression/Compression — dead, since every
original* field it reads is assigned solely in commented-out lines. The
constructor still runs it, so the initial state is unchanged.
Closing the popup still cannot stop a decode already under way: neither
BitmapImage.SetSourceAsync nor the video path takes a cancellation token. Only
the retention is fixed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Move the SendFilesPopup thumbnails off the models
StorageMedia._preview was assigned in four places and released in none, and
the models outlive the popup — SendFileExecute hands them to the send loop —
so every decoded thumbnail stayed alive for the whole send. A photo costs
roughly a megabyte and a video twice that, so a large drop retained tens of
megabytes with nothing left to free them.
Preview/Refresh/RefreshAsync are gone. The decode moves verbatim into
StorageThumbnailCache, owned by the popup, which pushes into the ImageBrush
instead of being pulled through a binding. Entries are dropped as their album
container is recycled, so the live set is now bounded by what the ListView has
realized rather than by how many files were picked, and cleared outright on
unload.
The cache also coalesces: Refresh() was async void fired from the Preview
getter with no in-flight guard, and _preview stayed null across the await, so
every container that asked before the first decode returned started another
decode of the same file — and the album panel rebuilds all of its children on
every UpdatePanel.
A decode that finishes after its entry was released, invalidated or cleared is
dropped rather than cached. That is what stops a post-recycle result from
resurrecting a bitmap nothing is left to release, and a pre-crop image from
landing after Invalidate.
The StorageVideo.Refresh override goes with it. It re-ran LoadPreview() after
a crop, which only feeds MaxCompression/Compression — dead, since every
original* field it reads is assigned solely in commented-out lines. The
constructor still runs it, so the initial state is unchanged.
Closing the popup still cannot stop a decode already under way: neither
BitmapImage.SetSourceAsync nor the video path takes a cancellation token. Only
the retention is fixed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • f094679 • 4 files, +371/-38
Show SendFilesPopup before the files have been typed
Dropping a lot of files left the user staring at nothing. Every file was
probed before the popup was constructed, one after another, and a probe is a
file open plus a header decode for photos or a whole ffmpeg open for video and
audio. Nothing was on screen until the last one finished.
StorageMedia.ProbeAsync types files concurrently, capped so a large drop cannot
open hundreds of decoders at once, and reports each result with its original
index as it lands. Every await in the chain captures the caller's context, so
the callback arrives on the UI thread.
SendFileExecute no longer probes. Both overloads funnel into SendFilesAsync,
which takes either already-typed items or raw files; with files it builds the
popup empty and starts probing from the Loaded handler. That hook is load
bearing: OpenAsync queues behind any other dialog and only creates its closing
task once it reaches the front, so a Hide from a result that resolved earlier
would have had nothing to close.
Results are buffered and flushed on a low priority dispatch, so everything
resolving within one UI turn is appended by a single AddRange -- one
CollectionChanged, one UpdateView, one UpdatePanel. Each batch is sorted by
original index, so the picked order survives whenever probing is fast enough to
land in one flush, which is the common case.
The permission and size guard moved rather than disappeared. One Validating
function holds the original messages: a loop up front for already-typed items,
a callback for probed ones. The first failure cancels probing and closes the
popup, and the error is raised after OpenAsync returns, which is already where
the caption is restored.
Three consequences. The title counts what is still coming rather than what has
landed, so it shows the drop's real size instead of ticking up, and stays on
the Files declension until types are known. The requested media/files mode
cannot be resolved against an empty list, so UpdateView settles it as the first
items arrive, until the user picks a mode themselves. Send is disabled while
probing and Accept returns early, since Enter bypasses the button and sending
half a drop would silently discard the rest.
The batch CreateAsync overload keeps its serial loop for the callers that still
need it, but its try-catch moved inside the loop: one unreadable file used to
discard every file after it.
Known edge: if every file fails to probe the popup appears briefly and then
closes itself, where before it never appeared. The alternative is waiting for
the first result, which is the stall this removes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Show SendFilesPopup before the files have been typed
Dropping a lot of files left the user staring at nothing. Every file was
probed before the popup was constructed, one after another, and a probe is a
file open plus a header decode for photos or a whole ffmpeg open for video and
audio. Nothing was on screen until the last one finished.
StorageMedia.ProbeAsync types files concurrently, capped so a large drop cannot
open hundreds of decoders at once, and reports each result with its original
index as it lands. Every await in the chain captures the caller's context, so
the callback arrives on the UI thread.
SendFileExecute no longer probes. Both overloads funnel into SendFilesAsync,
which takes either already-typed items or raw files; with files it builds the
popup empty and starts probing from the Loaded handler. That hook is load
bearing: OpenAsync queues behind any other dialog and only creates its closing
task once it reaches the front, so a Hide from a result that resolved earlier
would have had nothing to close.
Results are buffered and flushed on a low priority dispatch, so everything
resolving within one UI turn is appended by a single AddRange -- one
CollectionChanged, one UpdateView, one UpdatePanel. Each batch is sorted by
original index, so the picked order survives whenever probing is fast enough to
land in one flush, which is the common case.
The permission and size guard moved rather than disappeared. One Validating
function holds the original messages: a loop up front for already-typed items,
a callback for probed ones. The first failure cancels probing and closes the
popup, and the error is raised after OpenAsync returns, which is already where
the caption is restored.
Three consequences. The title counts what is still coming rather than what has
landed, so it shows the drop's real size instead of ticking up, and stays on
the Files declension until types are known. The requested media/files mode
cannot be resolved against an empty list, so UpdateView settles it as the first
items arrive, until the user picks a mode themselves. Send is disabled while
probing and Accept returns early, since Enter bypasses the button and sending
half a drop would silently discard the rest.
The batch CreateAsync overload keeps its serial loop for the callers that still
need it, but its try-catch moved inside the loop: one unreadable file used to
discard every file after it.
Known edge: if every file fails to probe the popup appears briefly and then
closes itself, where before it never appeared. The alternative is waiting for
the first result, which is the stall this removes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • a8f394e • 5 files, +172/-90
Give SendFilesPopup one input shape
Showing the popup before the files were typed left it with a three step
construction protocol: build it with an empty list, set Validating, then call
Probe from the caller's Loaded handler, all driven by a SendFilesAsync that
took an items/files pair where exactly one was allowed to be null. Miss a step
and you get a silently empty popup or an unguarded send, and nothing enforces
it. The two construction sites differ enough for that to be a real trap -- the
edit path passes a single already-typed item, has no guard at all, and reads
Items[0] back the moment the popup closes.
StorageMediaSource is now the only thing the popup is given. FromMedia exposes
everything through Ready; FromFiles leaves Ready empty and delivers through
LoadAsync. Count is known up front either way.
The constructor seeds Items from Ready rather than loading them, so the edit
path still has its item before the popup opens and Items[0] cannot throw, and
the popup calls LoadAsync from its own Loaded, which is a no-op when the source
is already complete. Callers can no longer forget to start it.
Two things fall out. The expected count comes from the source at construction
instead of being patched in later, so the title states the size of a drop from
the first frame. And the up-front guard loop runs over Ready, which is empty
for files, so the last of the two-flavour branching disappeared rather than
moving somewhere else.
The guard is a constructor argument instead of a settable property. The edit
path passes null, which is honest: that item's permissions were checked when it
was first sent.
SendFileExecute's media overload now takes IReadOnlyList, since IList does not
convert to it and the source needs the read-only form. Its one caller passes an
array.
The constructor's Logger.Info line enumerates ready items, so on the drop path
it would have gone blank. It records the pending count instead -- that line
ships with crash reports.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Give SendFilesPopup one input shape
Showing the popup before the files were typed left it with a three step
construction protocol: build it with an empty list, set Validating, then call
Probe from the caller's Loaded handler, all driven by a SendFilesAsync that
took an items/files pair where exactly one was allowed to be null. Miss a step
and you get a silently empty popup or an unguarded send, and nothing enforces
it. The two construction sites differ enough for that to be a real trap -- the
edit path passes a single already-typed item, has no guard at all, and reads
Items[0] back the moment the popup closes.
StorageMediaSource is now the only thing the popup is given. FromMedia exposes
everything through Ready; FromFiles leaves Ready empty and delivers through
LoadAsync. Count is known up front either way.
The constructor seeds Items from Ready rather than loading them, so the edit
path still has its item before the popup opens and Items[0] cannot throw, and
the popup calls LoadAsync from its own Loaded, which is a no-op when the source
is already complete. Callers can no longer forget to start it.
Two things fall out. The expected count comes from the source at construction
instead of being patched in later, so the title states the size of a drop from
the first frame. And the up-front guard loop runs over Ready, which is empty
for files, so the last of the two-flavour branching disappeared rather than
moving somewhere else.
The guard is a constructor argument instead of a settable property. The edit
path passes null, which is honest: that item's permissions were checked when it
was first sent.
SendFileExecute's media overload now takes IReadOnlyList, since IList does not
convert to it and the source needs the read-only form. Its one caller passes an
array.
The constructor's Logger.Info line enumerates ready items, so on the drop path
it would have gone blank. It records the pending count instead -- that line
ships with crash reports.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • b4cfc42 • 4 files, +139/-49
Grow SendFilesPopup albums instead of rebuilding them
CompareItems compared album contents, so an album that gained a photo was a
different item and the diff removed and re-inserted it. That recycled the
container, and the recycle handler releases the album's thumbnails on the way
out -- so every batch of arriving photos re-decoded every thumbnail the album
already had. The remeasure was the visible symptom; this was the cost.
ChatDiffHandler shows the intended contract: CompareItems is identity and
UpdateItem carries the new content across. StorageAlbum now has an Ordinal, its
position among the albums of a view, and that is what CompareItems compares. An
album that gains a photo is the same album with new contents, and its container
survives.
UpdateItem moves the new media onto the retained instance and refreshes the
realized panel. Before it only invalidated layout, which meant that whenever it
did fire the panel went on rendering the old contents -- latent, since the old
CompareItems almost never let it fire.
StorageAlbumPanel.UpdateMessage reuses its children rather than clearing them
and allocating a Button per item with a fresh Click subscription. Growth only
appends now, and a surviving item keeps the thumbnail it already had, because
changing Button.Content updates the template root's DataContext instead of
rebuilding it.
Remove_Click invalidates the removed item's thumbnail, since a container
recycling underneath it no longer does.
Also restores the width and height diagnostic. The constructor's Logger.Info
line names each item's dimensions, which is what album layout bugs get
diagnosed from, and the drop path had reduced it to a count because nothing is
typed that early. It now logs the pending count there and logs the dimensions
again once everything has landed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 411fbcd • 5 files, +15/-6
Stop leading text inputs with the packaged emoji font
EmojiTextThemeFontFamily was built emoji-first, so every text input resolved
against ms-appx:///Assets/Emoji/*.ttf before falling back to the text font. The
editor resolves the font once per run and breaks runs at every space, so that
cost about a millisecond per word: pasting 17,000 characters into the composer
froze the UI for 2.7 seconds. Measured identical through SetText and through
RichEdit's own paste, and identical for one line or for hundreds, so it is the
font resolution rather than the insertion.
Leading with the text font instead costs the emojis whose base character it
already covers - keycaps, the copyright and trademark signs and so on -
rendering as plain glyphs. That is confined to input controls: message bubbles
render from XamlAutoFontFamily, which is untouched, so what gets sent and what
gets displayed are both unaffected.
Only three styles used the resource, all of them text input, and four further
inputs were leading with ContentControlThemeFontFamily, which is emoji-first
too and so was just as slow. They all take EmojiTextThemeFontFamily now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Grow SendFilesPopup albums instead of rebuilding them
CompareItems compared album contents, so an album that gained a photo was a
different item and the diff removed and re-inserted it. That recycled the
container, and the recycle handler releases the album's thumbnails on the way
out -- so every batch of arriving photos re-decoded every thumbnail the album
already had. The remeasure was the visible symptom; this was the cost.
ChatDiffHandler shows the intended contract: CompareItems is identity and
UpdateItem carries the new content across. StorageAlbum now has an Ordinal, its
position among the albums of a view, and that is what CompareItems compares. An
album that gains a photo is the same album with new contents, and its container
survives.
UpdateItem moves the new media onto the retained instance and refreshes the
realized panel. Before it only invalidated layout, which meant that whenever it
did fire the panel went on rendering the old contents -- latent, since the old
CompareItems almost never let it fire.
StorageAlbumPanel.UpdateMessage reuses its children rather than clearing them
and allocating a Button per item with a fresh Click subscription. Growth only
appends now, and a surviving item keeps the thumbnail it already had, because
changing Button.Content updates the template root's DataContext instead of
rebuilding it.
Remove_Click invalidates the removed item's thumbnail, since a container
recycling underneath it no longer does.
Also restores the width and height diagnostic. The constructor's Logger.Info
line names each item's dimensions, which is what album layout bugs get
diagnosed from, and the drop path had reduced it to a count because nothing is
typed that early. It now logs the pending count there and logs the dimensions
again once everything has landed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 411fbcd • 5 files, +15/-6
Stop leading text inputs with the packaged emoji font
EmojiTextThemeFontFamily was built emoji-first, so every text input resolved
against ms-appx:///Assets/Emoji/*.ttf before falling back to the text font. The
editor resolves the font once per run and breaks runs at every space, so that
cost about a millisecond per word: pasting 17,000 characters into the composer
froze the UI for 2.7 seconds. Measured identical through SetText and through
RichEdit's own paste, and identical for one line or for hundreds, so it is the
font resolution rather than the insertion.
Leading with the text font instead costs the emojis whose base character it
already covers - keycaps, the copyright and trademark signs and so on -
rendering as plain glyphs. That is confined to input controls: message bubbles
render from XamlAutoFontFamily, which is untouched, so what gets sent and what
gets displayed are both unaffected.
Only three styles used the resource, all of them text input, and four further
inputs were leading with ContentControlThemeFontFamily, which is emoji-first
too and so was just as slow. They all take EmojiTextThemeFontFamily now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • 0827fe2 • 2 files, +59/-0
Add a profiler for scoped timings
Begin and End are [Conditional("INSTRUMENTATION")] like Instrumentation.Register,
so a probe can sit permanently at a call site and compiles out of every build
that doesn't define the symbol. A helper returning a timestamp cannot do that:
the reading itself would stay in the hot path, which is where these get placed.
Scopes nest and are reported indented, so an outer scope shows what it contains.
End unwinds to its own label, so a return between a pair reports the abandoned
scopes rather than silently charging the rest of the run to them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • fc25f0d • 1 files, +122/-109
Probe the document once instead of walking it on every keystroke
UpdateFormat, UpdateCustomEmoji and UpdateBlocks each walked the whole document
on every keystroke. The editor breaks character format runs at every space, so
the first of those was O(words) round trips into the text host per character
typed.
TOM reports a property as undefined when it isn't uniform over a range, so a
single read over the whole story says whether there is anything to walk for at
all: no hidden text means no custom emoji, a uniform SpaceAfter of zero means no
blockquote, and a document already at the regular size has no font size to fix.
Alongside that: the ranges these walk with are kept and moved rather than
allocated on each pass, since every ITextRange is a COM object; applying
entities in SetText reuses one range instead of one per entity; and UpdateBlocks
removed stale blockquote decorations with an index loop that skipped every other
element.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • dc8291a • 1 files, +26/-6
Stop allocating per character in the emoticon replacement
OnCharacterReceived runs for every character typed. It built a byte array and a
string just to recover the first char of the code point, then used Max and a
capturing FirstOrDefault over the candidates. The first char comes straight off
the code point now and the two LINQ calls are loops.
The Consolas check moved behind the candidate lookup, so a keystroke that can't
end an emoticon no longer pays for a call into the text host, and EndsWith is
ordinal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • eb2e555 • 1 files, +14/-12
Don't recreate every custom emoji source on each keystroke
UpdateEntities assigned a new CustomEmojiFileSource to every player on every
text change. Each one sends getCustomEmojiStickers from its constructor and
re-runs AnimatedImage.Load. It only assigns when the id actually differs now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Add a profiler for scoped timings
Begin and End are [Conditional("INSTRUMENTATION")] like Instrumentation.Register,
so a probe can sit permanently at a call site and compiles out of every build
that doesn't define the symbol. A helper returning a timestamp cannot do that:
the reading itself would stay in the hot path, which is where these get placed.
Scopes nest and are reported indented, so an outer scope shows what it contains.
End unwinds to its own label, so a return between a pair reports the abandoned
scopes rather than silently charging the rest of the run to them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • fc25f0d • 1 files, +122/-109
Probe the document once instead of walking it on every keystroke
UpdateFormat, UpdateCustomEmoji and UpdateBlocks each walked the whole document
on every keystroke. The editor breaks character format runs at every space, so
the first of those was O(words) round trips into the text host per character
typed.
TOM reports a property as undefined when it isn't uniform over a range, so a
single read over the whole story says whether there is anything to walk for at
all: no hidden text means no custom emoji, a uniform SpaceAfter of zero means no
blockquote, and a document already at the regular size has no font size to fix.
Alongside that: the ranges these walk with are kept and moved rather than
allocated on each pass, since every ITextRange is a COM object; applying
entities in SetText reuses one range instead of one per entity; and UpdateBlocks
removed stale blockquote decorations with an index loop that skipped every other
element.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • dc8291a • 1 files, +26/-6
Stop allocating per character in the emoticon replacement
OnCharacterReceived runs for every character typed. It built a byte array and a
string just to recover the first char of the code point, then used Max and a
capturing FirstOrDefault over the candidates. The first char comes straight off
the code point now and the two LINQ calls are loops.
The Consolas check moved behind the candidate lookup, so a keystroke that can't
end an emoticon no longer pays for a call into the text host, and EndsWith is
ordinal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • eb2e555 • 1 files, +14/-12
Don't recreate every custom emoji source on each keystroke
UpdateEntities assigned a new CustomEmojiFileSource to every player on every
text change. Each one sends getCustomEmojiStickers from its constructor and
re-runs AnimatedImage.Load. It only assigns when the id actually differs now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/composer-typing-performance • 3ea56fa • 1 files, +17/-2
Stop splitting the whole message to find the inline bot
SearchInlineBotResults runs on every keystroke, and split the entire message on
spaces to look at its first word - plus two substrings before that - all of it
dead unless an inline bot is being addressed. That check comes first now, and
the first word is taken with IndexOf.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • e758fe9 • 1 files, +57/-32
Don't ask for a link preview when the text can't hold a URL
CheckMessageBoxEmpty sent getLinkPreview on every keystroke whatever the text:
the whole message serialized to JSON, through the TDLib queue, and a dispatcher
hop back into another full GetText to check staleness, plus the four whole
string copies Format() makes.
A URL always carries either an explicit scheme or a dot before its top level
domain, so a scan that can be wrong about a link being there but never about one
not being there now gates all of it. The clear-the-preview block that appeared
twice became one method.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 208b8e6 • 2 files, +76/-8
Paste through a batched insert, and don't relabel a link that is one already
The insert and the caret move that follows it each lay the document out, so they
run inside BatchDisplayUpdates.
Pasting a URL over a selection labelled it by way of SetText, which rewrote the
selection with its own text so it could re-derive a range from offsets. It sets
the link on the range directly instead, which drops two side effects: SetTextImpl
ran the text through IsLongerThanMaxLength, so replacing a selection with itself
could be truncated near the limit, and it destroyed anything hidden inside the
selection.
It no longer labels a selection that is a link already, whether it carries one or
is one. Nothing marks a typed URL as a link while composing - entities are
resolved at send - so replacing one link with another turned the old URL into the
label of a link pointing at the new one.
IsValidUrl rejects on whitespace before handing the string to the entity parser,
since it only accepts an entity spanning the whole string anyway.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 8fd7857 • 1 files, +122/-0
Write down what the composer review turned up
Unfixed findings in FormattedTextBox and ChatTextBox, ordered by severity, with
the ones fixed while investigating listed at the end so the file reads as the
current state. Check an item off in the same commit as its fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Stop splitting the whole message to find the inline bot
SearchInlineBotResults runs on every keystroke, and split the entire message on
spaces to look at its first word - plus two substrings before that - all of it
dead unless an inline bot is being addressed. That check comes first now, and
the first word is taken with IndexOf.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • e758fe9 • 1 files, +57/-32
Don't ask for a link preview when the text can't hold a URL
CheckMessageBoxEmpty sent getLinkPreview on every keystroke whatever the text:
the whole message serialized to JSON, through the TDLib queue, and a dispatcher
hop back into another full GetText to check staleness, plus the four whole
string copies Format() makes.
A URL always carries either an explicit scheme or a dot before its top level
domain, so a scan that can be wrong about a link being there but never about one
not being there now gates all of it. The clear-the-preview block that appeared
twice became one method.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 208b8e6 • 2 files, +76/-8
Paste through a batched insert, and don't relabel a link that is one already
The insert and the caret move that follows it each lay the document out, so they
run inside BatchDisplayUpdates.
Pasting a URL over a selection labelled it by way of SetText, which rewrote the
selection with its own text so it could re-derive a range from offsets. It sets
the link on the range directly instead, which drops two side effects: SetTextImpl
ran the text through IsLongerThanMaxLength, so replacing a selection with itself
could be truncated near the limit, and it destroyed anything hidden inside the
selection.
It no longer labels a selection that is a link already, whether it carries one or
is one. Nothing marks a typed URL as a link while composing - entities are
resolved at send - so replacing one link with another turned the old URL into the
label of a link pointing at the new one.
IsValidUrl rejects on whitespace before handing the string to the entity parser,
since it only accepts an entity spanning the whole string anyway.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/composer-typing-performance • 8fd7857 • 1 files, +122/-0
Write down what the composer review turned up
Unfixed findings in FormattedTextBox and ChatTextBox, ordered by severity, with
the ones fixed while investigating listed at the end so the file reads as the
current state. Check an item off in the same commit as its fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/exception-reporting • 01940da • 1 files, +36/-17
Read the rest of the stowed exception record
GetStowedException2 required ExceptionForm 1 and returned null otherwise, so
three fields the record always carries went unused:
- ResultCode, the HRESULT the error was stowed with, before propagation
flattened it to E_FAIL. This is the field that actually distinguishes one
failure from another.
- ThreadId, the thread it originated on, which is not necessarily the one whose
stack ends up in the report.
- ErrorText, which form 2 carries instead of a stack, and which was discarded
along with the whole record.
It also returned null when no frame resolved a module base, dropping the above
and the nested record with them. Now a FatalError is returned either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Read the rest of the stowed exception record
GetStowedException2 required ExceptionForm 1 and returned null otherwise, so
three fields the record always carries went unused:
- ResultCode, the HRESULT the error was stowed with, before propagation
flattened it to E_FAIL. This is the field that actually distinguishes one
failure from another.
- ThreadId, the thread it originated on, which is not necessarily the one whose
stack ends up in the report.
- ErrorText, which form 2 carries instead of a stack, and which was discarded
along with the whole record.
It also returned null when no frame resolved a module base, dropping the above
and the nested record with them. Now a FatalError is returned either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
👍1
UnigramDev/Unigram/voice-record-feedback • e6b041a • 1 files, +30/-3
Say something when voice recording cannot be engaged
CheckDeviceAccessAsync swallowed every exception from MediaCapture.InitializeAsync
and returned false regardless, so a device that fails to initialise - absent,
already in use, or a driver that refuses - was indistinguishable from the ordinary
"consent prompt raised, press again" case. The button simply did nothing, with no
message and no log entry, which is what #2093 asked about in 2020. The catch in
CheckAccessAsync above it still carried a "TODO: notify user".
Both now log and surface the error. Strings.UnknownErrorCode already exists, so no
new resource is needed.
Release() also gained a log line on the branch that drops a locked-mode send.
Nobody has reproduced #3262, and that is the only path which can swallow it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Say something when voice recording cannot be engaged
CheckDeviceAccessAsync swallowed every exception from MediaCapture.InitializeAsync
and returned false regardless, so a device that fails to initialise - absent,
already in use, or a driver that refuses - was indistinguishable from the ordinary
"consent prompt raised, press again" case. The button simply did nothing, with no
message and no log entry, which is what #2093 asked about in 2020. The catch in
CheckAccessAsync above it still carried a "TODO: notify user".
Both now log and surface the error. Strings.UnknownErrorCode already exists, so no
new resource is needed.
Release() also gained a log line on the branch that drops a locked-mode send.
Nobody has reproduced #3262, and that is the only path which can swallow it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram