UnigramDev/Unigram/develop • a10164e • 2 files, +41/-13
Don't recreate the Direct3D device from the device-lost callback (#3333)
PlaceholderImageHelper watches its D3D device with RegisterDeviceRemovedEvent
and recreated the device straight from the threadpool wait callback. That runs
while the display driver is still resetting, and D3D11CreateDevice at that
moment faults inside the vendor user-mode driver (reported by crash telemetry
on 12.9, on two different Intel UMDs).
The device is now rebuilt lazily instead: PlaceholderHelper already calls
HandleDeviceLost() on every access to the singleton, so the next use recreates
it from the UI thread. HandleDeviceLost tolerates a null device, which is the
state left behind when creation failed.
Two related lifetime fixes in the same path:
- CreateDeviceResources released nothing before calling D3D11CreateDevice.
com_ptr::put() overwrites without releasing, so recreating over the removed
device stranded it and everything the driver held behind it.
- PlaceholderImageHelper::Close() called StopWatchingCurrentDevice(), which
closes the threadpool wait but does not wait for a callback that is already
running -- the callback then reached RaiseDeviceLostEvent on an object that
was being destroyed. The drain that already existed in ~DeviceLostHelper is
now a Shutdown() method that Close() calls, and it was dead code before
because Close() had already cleared the handle it checks.
Not built: no UWP/.NET Native build environment here.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 8b5b974 • 2 files, +50/-5
Log why the video swap chain failed, and don't crash when it does (#3334)
AsyncMediaPlayerSwapChain::Create() swallowed every failure and returned false,
so the only trace left was SwapChainOptions() throwing "You must wait for the
VideoView to be loaded before calling GetSwapChainOptions()" -- a message
inherited from LibVLCSharp that describes a lifecycle mistake this code cannot
make. The real cause is that one of the Direct3D calls failed, and nothing
recorded which one. That throw crossed a XAML Loaded handler (gallery video,
stories, the music player all construct AsyncMediaPlayer there) and became an
unhandled exception, reported by crash telemetry on 12.8.1 and 12.9.
Create() now names each stage as it goes and logs the stage, the HRESULT and
the message, so the next report says which driver call failed. The thrown
message says what actually happened.
The player no longer fails construction over it: the Direct3D device can
legitimately be unavailable while the display driver is resetting, and there is
nothing the caller could do about it. When the swap chain is not loaded the
player is built with --no-video instead, so audio still plays and the failure
is in the log rather than in a crash.
Also releases m_d3d11Device/m_deviceContext between adapter attempts --
com_ptr::put() overwrites without releasing, so a failed attempt could strand a
device.
Not built: no UWP/.NET Native build environment here.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Don't recreate the Direct3D device from the device-lost callback (#3333)
PlaceholderImageHelper watches its D3D device with RegisterDeviceRemovedEvent
and recreated the device straight from the threadpool wait callback. That runs
while the display driver is still resetting, and D3D11CreateDevice at that
moment faults inside the vendor user-mode driver (reported by crash telemetry
on 12.9, on two different Intel UMDs).
The device is now rebuilt lazily instead: PlaceholderHelper already calls
HandleDeviceLost() on every access to the singleton, so the next use recreates
it from the UI thread. HandleDeviceLost tolerates a null device, which is the
state left behind when creation failed.
Two related lifetime fixes in the same path:
- CreateDeviceResources released nothing before calling D3D11CreateDevice.
com_ptr::put() overwrites without releasing, so recreating over the removed
device stranded it and everything the driver held behind it.
- PlaceholderImageHelper::Close() called StopWatchingCurrentDevice(), which
closes the threadpool wait but does not wait for a callback that is already
running -- the callback then reached RaiseDeviceLostEvent on an object that
was being destroyed. The drain that already existed in ~DeviceLostHelper is
now a Shutdown() method that Close() calls, and it was dead code before
because Close() had already cleared the handle it checks.
Not built: no UWP/.NET Native build environment here.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 8b5b974 • 2 files, +50/-5
Log why the video swap chain failed, and don't crash when it does (#3334)
AsyncMediaPlayerSwapChain::Create() swallowed every failure and returned false,
so the only trace left was SwapChainOptions() throwing "You must wait for the
VideoView to be loaded before calling GetSwapChainOptions()" -- a message
inherited from LibVLCSharp that describes a lifecycle mistake this code cannot
make. The real cause is that one of the Direct3D calls failed, and nothing
recorded which one. That throw crossed a XAML Loaded handler (gallery video,
stories, the music player all construct AsyncMediaPlayer there) and became an
unhandled exception, reported by crash telemetry on 12.8.1 and 12.9.
Create() now names each stage as it goes and logs the stage, the HRESULT and
the message, so the next report says which driver call failed. The thrown
message says what actually happened.
The player no longer fails construction over it: the Direct3D device can
legitimately be unavailable while the display driver is resetting, and there is
nothing the caller could do about it. When the swap chain is not loaded the
player is built with --no-video instead, so audio still plays and the failure
is in the log rather than in a crash.
Also releases m_d3d11Device/m_deviceContext between adapter attempts --
com_ptr::put() overwrites without releasing, so a failed attempt could strand a
device.
Not built: no UWP/.NET Native build environment here.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/develop • 9b8ba30 • 1 files, +7/-1
Don't animate a deferred template part from SettingsButton's Disabled state (#3336)
* Defer DescriptionPresenter with Lazy instead of dropping the deferral
x:DeferLoadStrategy="Lazy" realizes the element when something references it,
including a visual state, so the Disabled storyboard resolves without the part
being materialized up front. x:Load does not, which is what made the target
unresolvable. The stock TextBox template deferrs HeaderContentPresenter the same
way for the same reason.
Keeps the deferral the previous commit had given up, and the disabled appearance
is still what it was before this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • a04d5d4 • 1 files, +7/-0
Handle an instant view table with no rows (#3335)
* Handle an instant view table with no rows
ProcessTable computed its column count with table.Cells.Max(...), which throws
InvalidOperationException: Sequence contains no elements when the table has no
rows at all. It reached OnApplyTemplate through InstantContent.UpdateView, so
the whole message failed to render. Reported by crash telemetry on 12.9.1.
There is no grid to build for an empty table, so render just the caption --
which is what the method already falls back to, and callers of ProcessBlock all
handle a null return.
Not built: no UWP/.NET Native build environment here. Verified the file still
parses with Roslyn.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Don't animate a deferred template part from SettingsButton's Disabled state (#3336)
* Defer DescriptionPresenter with Lazy instead of dropping the deferral
x:DeferLoadStrategy="Lazy" realizes the element when something references it,
including a visual state, so the Disabled storyboard resolves without the part
being materialized up front. x:Load does not, which is what made the target
unresolvable. The stock TextBox template deferrs HeaderContentPresenter the same
way for the same reason.
Keeps the deferral the previous commit had given up, and the disabled appearance
is still what it was before this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • a04d5d4 • 1 files, +7/-0
Handle an instant view table with no rows (#3335)
* Handle an instant view table with no rows
ProcessTable computed its column count with table.Cells.Max(...), which throws
InvalidOperationException: Sequence contains no elements when the table has no
rows at all. It reached OnApplyTemplate through InstantContent.UpdateView, so
the whole message failed to render. Reported by crash telemetry on 12.9.1.
There is no grid to build for an empty table, so render just the caption --
which is what the method already falls back to, and callers of ProcessBlock all
handle a null return.
Not built: no UWP/.NET Native build environment here. Verified the file still
parses with Roslyn.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Ajaxy/telegram-tt/master • 383d432 • 3 files, +5/-5
12.0.38
Ajaxy/telegram-tt/master • ca98574 • 10 files, +144/-2
Status Picker: Add roll effect (#7115)
Ajaxy/telegram-tt/master • ac04936 • 1 files, +1/-1
Poll: Fix sending quizes with no solution text (#7116)
Ajaxy/telegram-tt/master • af03e83 • 30 files, +1916/-463
GramJS: Protocol hardening (#7098)
Ajaxy/telegram-tt/master • 9a7b740 • 1 files, +7/-1
Albums: Fix separators for albums (#7119)
Ajaxy/telegram-tt/master • a3faeb0 • 4 files, +239/-218
[Dev] Bump Vite (#7120)
Ajaxy/telegram-tt/master • 549ac26 • 1 files, +1/-1
Composer: Fix sending forward on Enter (#7117)
Ajaxy/telegram-tt/master • c8049f1 • 44 files, +1309/-618
Middle Header Panes: Redesign + Refactoring (#7096)
Co-authored-by: zubiden <19638254+zubiden@users.noreply.github.com>
Co-authored-by: Alexander Zinchuk <alx.zinchuk@gmail.com>
Ajaxy/telegram-tt/master • edaa932 • 1 files, +10/-3
Radial Pattern Background: Fix color (#7113)
Ajaxy/telegram-tt/master • c06de3a • 22 files, +398/-374
Left Search: Redesign (#7111)
Ajaxy/telegram-tt/master • 69bb990 • 50 files, +2236/-1028
Support server wallpapers (#7076)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: zubiden <19638254+zubiden@users.noreply.github.com>
Co-authored-by: Dmitry Kabanov <153344039+dmitrykabanovdev@users.noreply.github.com>
Ajaxy/telegram-tt/master • 2305f82 • 1 files, +6/-1
Audio Player: Fix transition animation (#7123)
Ajaxy/telegram-tt/master • eaa9956 • 3 files, +24/-12
Forward Recipient Picker: Support quick forward to saved messages (#7122)
Ajaxy/telegram-tt/master • 0cc7471 • 11 files, +80/-177
Manage Group Members: Fix search data (#7121)
#webz
12.0.38
Ajaxy/telegram-tt/master • ca98574 • 10 files, +144/-2
Status Picker: Add roll effect (#7115)
Ajaxy/telegram-tt/master • ac04936 • 1 files, +1/-1
Poll: Fix sending quizes with no solution text (#7116)
Ajaxy/telegram-tt/master • af03e83 • 30 files, +1916/-463
GramJS: Protocol hardening (#7098)
Ajaxy/telegram-tt/master • 9a7b740 • 1 files, +7/-1
Albums: Fix separators for albums (#7119)
Ajaxy/telegram-tt/master • a3faeb0 • 4 files, +239/-218
[Dev] Bump Vite (#7120)
Ajaxy/telegram-tt/master • 549ac26 • 1 files, +1/-1
Composer: Fix sending forward on Enter (#7117)
Ajaxy/telegram-tt/master • c8049f1 • 44 files, +1309/-618
Middle Header Panes: Redesign + Refactoring (#7096)
Co-authored-by: zubiden <19638254+zubiden@users.noreply.github.com>
Co-authored-by: Alexander Zinchuk <alx.zinchuk@gmail.com>
Ajaxy/telegram-tt/master • edaa932 • 1 files, +10/-3
Radial Pattern Background: Fix color (#7113)
Ajaxy/telegram-tt/master • c06de3a • 22 files, +398/-374
Left Search: Redesign (#7111)
Ajaxy/telegram-tt/master • 69bb990 • 50 files, +2236/-1028
Support server wallpapers (#7076)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: zubiden <19638254+zubiden@users.noreply.github.com>
Co-authored-by: Dmitry Kabanov <153344039+dmitrykabanovdev@users.noreply.github.com>
Ajaxy/telegram-tt/master • 2305f82 • 1 files, +6/-1
Audio Player: Fix transition animation (#7123)
Ajaxy/telegram-tt/master • eaa9956 • 3 files, +24/-12
Forward Recipient Picker: Support quick forward to saved messages (#7122)
Ajaxy/telegram-tt/master • 0cc7471 • 11 files, +80/-177
Manage Group Members: Fix search data (#7121)
#webz
🫡2
Ajaxy/telegram-tt/master • 6786a05 • 7 files, +49/-7
Wallpapers: Follow up (#7125)
Ajaxy/telegram-tt/master • c770a38 • 77 files, +1922/-1358
Settings: Redesign icons (#7124)
Ajaxy/telegram-tt/master • 9716ca2 • 2 files, +24/-11
UI: Fix blink on long first load (#7129)
Ajaxy/telegram-tt/master • 37f1364 • 24 files, +476/-301
Stickers: Migrate to TLottie (#7128)
Ajaxy/telegram-tt/master • 911c59c • 22 files, +221/-122
[chore] Avoid Record<string, unknown> (#7130)
Ajaxy/telegram-tt/master • 8ae788a • 3 files, +83/-123
Vite: Fix startup speed (#7131)
Ajaxy/telegram-tt/master • 1211dd5 • 7 files, +1326/-1791
Tauri: Bump dependencies (#7133)
Ajaxy/telegram-tt/master • 786f1ff • 8 files, +2/-87
Service Messages: Remove changelog (#7134)
Ajaxy/telegram-tt/master • c5a2bff • 1 files, +14/-6
Message List: Fix scroll restore (#7135)
Ajaxy/telegram-tt/master • d915b1b • 300 files, +2984/-2890
[Build]
#webz
Wallpapers: Follow up (#7125)
Ajaxy/telegram-tt/master • c770a38 • 77 files, +1922/-1358
Settings: Redesign icons (#7124)
Ajaxy/telegram-tt/master • 9716ca2 • 2 files, +24/-11
UI: Fix blink on long first load (#7129)
Ajaxy/telegram-tt/master • 37f1364 • 24 files, +476/-301
Stickers: Migrate to TLottie (#7128)
Ajaxy/telegram-tt/master • 911c59c • 22 files, +221/-122
[chore] Avoid Record<string, unknown> (#7130)
Ajaxy/telegram-tt/master • 8ae788a • 3 files, +83/-123
Vite: Fix startup speed (#7131)
Ajaxy/telegram-tt/master • 1211dd5 • 7 files, +1326/-1791
Tauri: Bump dependencies (#7133)
Ajaxy/telegram-tt/master • 786f1ff • 8 files, +2/-87
Service Messages: Remove changelog (#7134)
Ajaxy/telegram-tt/master • c5a2bff • 1 files, +14/-6
Message List: Fix scroll restore (#7135)
Ajaxy/telegram-tt/master • d915b1b • 300 files, +2984/-2890
[Build]
#webz
🫡2
UnigramDev/Unigram/develop • 390bfc8 • 5 files, +44/-62
Take the manager locks off the event and callback paths
winrt::event synchronises itself, so m_lock only served to serialise callbacks
against subscribe and unsubscribe — at the price of a lock order inversion with
the managed side, which unsubscribes from inside its own lock while a tgcalls
thread holds this one inside managed code that takes that same lock. Nothing
closed the cycle today, but it was there to be closed.
VoipManager keeps no lock at all now. VoipGroupManager keeps one for the only
state that is really shared, the encrypt and decrypt delegates, and copies them
out rather than holding it across the call: the managed side blocks on a TDLib
round trip in there, and it was doing that while holding the same mutex as every
audio level update and every subscribe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • d94416d • 5 files, +22/-3
Stop both managers from their destructor
Stop is what the managed side calls, but nothing guarantees every path gets
there, and destroying a manager without it skips tgcalls' own teardown and
leaves a screencast still capturing loopback audio. Stop is idempotent, so
calling it again from the destructor costs nothing when it already ran.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • d75db13 • 2 files, +5/-4
Use std::mutex in the loopback capture
winrt::slim_mutex is not deprecated, but every other mutex in Telegram.Native
and Telegram.Native.Calls is std::mutex and this one is uncontended anyway.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • b476751 • 4 files, +57/-41
Cut the per-frame interop out of the group call callbacks
The E2E path rebuilt its result with a push_back loop over an IIterator, which
is two COM calls per byte on every frame of a conference call; one GetMany into
a pre-sized buffer does it instead. Audio levels appended to the IVector one
participant at a time, ten times a second for the length of the call, and are
now filled into a reserved vector and handed over whole.
Same treatment for ReceiveSignalingData. SetRequestedVideoChannels stops
building its vector when there is no instance to give it to, and Protocol's
comparator stops copying both strings on every comparison.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Take the manager locks off the event and callback paths
winrt::event synchronises itself, so m_lock only served to serialise callbacks
against subscribe and unsubscribe — at the price of a lock order inversion with
the managed side, which unsubscribes from inside its own lock while a tgcalls
thread holds this one inside managed code that takes that same lock. Nothing
closed the cycle today, but it was there to be closed.
VoipManager keeps no lock at all now. VoipGroupManager keeps one for the only
state that is really shared, the encrypt and decrypt delegates, and copies them
out rather than holding it across the call: the managed side blocks on a TDLib
round trip in there, and it was doing that while holding the same mutex as every
audio level update and every subscribe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • d94416d • 5 files, +22/-3
Stop both managers from their destructor
Stop is what the managed side calls, but nothing guarantees every path gets
there, and destroying a manager without it skips tgcalls' own teardown and
leaves a screencast still capturing loopback audio. Stop is idempotent, so
calling it again from the destructor costs nothing when it already ran.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • d75db13 • 2 files, +5/-4
Use std::mutex in the loopback capture
winrt::slim_mutex is not deprecated, but every other mutex in Telegram.Native
and Telegram.Native.Calls is std::mutex and this one is uncontended anyway.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • b476751 • 4 files, +57/-41
Cut the per-frame interop out of the group call callbacks
The E2E path rebuilt its result with a push_back loop over an IIterator, which
is two COM calls per byte on every frame of a conference call; one GetMany into
a pre-sized buffer does it instead. Audio levels appended to the IVector one
participant at a time, ten times a second for the length of the call, and are
now filled into a reserved vector and handed over whole.
Same treatment for ReceiveSignalingData. SetRequestedVideoChannels stops
building its vector when there is no instance to give it to, and Protocol's
comparator stops copying both strings on every comparison.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/develop • 278ca6e • 4 files, +61/-42
Clear the remaining hygiene items in the call managers
The tgcalls version registrations had internal linkage but lived in a header, so
a second includer would have registered every version twice; they move into
VoipManager.cpp. The #ifndef _WIN32 arm of the config initialiser could never
have compiled and is gone.
A screencast runs a second group manager alongside the main one and both opened
tgcalls_group.txt, interleaving into one file; the screencast writes its own now.
EmitJoinPayload no longer completes when there is no instance to emit from. The
empty payload it used to hand back only got the caller as far as a join the
server would reject, and it did it on the calling thread rather than the one the
success path answers on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 5fb96f2 • 5 files, +309/-1
Base service message for community add/remove
UnigramDev/Unigram/develop • 03513e4 • 2 files, +22/-4
Let a null incoming video sink reach tgcalls
Skipping out early on null was wrong twice. It left m_incomingVideoOutput
pointing at the sink the caller had just asked to detach, and it swallowed the
one thing a null is good for: setIncomingVideoOutput also assigns _currentSink,
which every newly negotiated video channel is handed on creation, so a channel
appearing after the detach re-attached the old sink.
Null flows through as an empty shared_ptr now. The dedupe check handles it
without a special case, since two nulls in a row compare equal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 16ee8b2 • 2 files, +26/-2
Set responseTimestamp on every broadcast part status
NotReady is precisely the status tgcalls reads responseTimestamp on, to decide
where to restart a stream that has not begun yet, and it was only ever set on
the Success path — so it arrived as zero. The caller had been supplying it all
along and we dropped it.
It is also the one timestamp in this API measured in seconds rather than
milliseconds; tgcalls multiplies it back up by 1000. We passed milliseconds
straight through, a thousand times too large. Converting at the tgcalls boundary
keeps the WinRT surface consistently milliseconds, which is what
requestCurrentTime already expects.
Found by re-reading the commit that started reporting empty parts as NotReady,
which is what made this reachable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Clear the remaining hygiene items in the call managers
The tgcalls version registrations had internal linkage but lived in a header, so
a second includer would have registered every version twice; they move into
VoipManager.cpp. The #ifndef _WIN32 arm of the config initialiser could never
have compiled and is gone.
A screencast runs a second group manager alongside the main one and both opened
tgcalls_group.txt, interleaving into one file; the screencast writes its own now.
EmitJoinPayload no longer completes when there is no instance to emit from. The
empty payload it used to hand back only got the caller as far as a join the
server would reject, and it did it on the calling thread rather than the one the
success path answers on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 5fb96f2 • 5 files, +309/-1
Base service message for community add/remove
UnigramDev/Unigram/develop • 03513e4 • 2 files, +22/-4
Let a null incoming video sink reach tgcalls
Skipping out early on null was wrong twice. It left m_incomingVideoOutput
pointing at the sink the caller had just asked to detach, and it swallowed the
one thing a null is good for: setIncomingVideoOutput also assigns _currentSink,
which every newly negotiated video channel is handed on creation, so a channel
appearing after the detach re-attached the old sink.
Null flows through as an empty shared_ptr now. The dedupe check handles it
without a special case, since two nulls in a row compare equal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 16ee8b2 • 2 files, +26/-2
Set responseTimestamp on every broadcast part status
NotReady is precisely the status tgcalls reads responseTimestamp on, to decide
where to restart a stream that has not begun yet, and it was only ever set on
the Success path — so it arrived as zero. The caller had been supplying it all
along and we dropped it.
It is also the one timestamp in this API measured in seconds rather than
milliseconds; tgcalls multiplies it back up by 1000. We passed milliseconds
straight through, a thousand times too large. Converting at the tgcalls boundary
keeps the WinRT surface consistently milliseconds, which is what
requestCurrentTime already expects.
Found by re-reading the commit that started reporting empty parts as NotReady,
which is what made this reachable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/develop • 30ce540 • 5 files, +502/-195
Close the lock leak in the ClientService critical sections
Thirteen Monitor.Enter/Monitor.Exit pairs had no try/finally, so anything that
threw in between leaked the lock for good. The worst is UpdateChatLastMessage,
which constructs a MessageAlbumLastMessageService while holding the Chat: one
throw there locks that chat forever, the next UpdateChatPosition for it blocks
the TDLib receive thread, and since that thread is the only one draining
td_receive, every update in the app stops arriving with no crash to show for it.
GetChatFolders takes lock (chat) from the UI thread, so the chat list wedges too.
All thirteen are plain lock blocks now. The ten synchronous ones are a straight
wrap. The three paging methods could not be, because await is illegal inside
lock — which is exactly why they were written with a hand-placed Monitor.Exit
before the await in the first place. Each now decides under the lock how much is
still to be loaded and either builds its result and returns inside the lock, or
falls out of it and awaits with nothing held. Same semantics, same lock ordering,
one exit path instead of three.
The MOCKUP blocks in those three methods moved but were left as they are. MOCKUP
is not defined in any configuration and the blocks reference an undefined index
variable, so they have not compiled in a long time; quietly repairing dead code
that cannot be tested does not belong in a lock-safety change.
UpdateChatDraftMessage was not in the review that prompted this — it turned up
grepping for Monitor. after converting the twelve that were listed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 533cbd9 • 3 files, +335/-147
Close the lock leak in the two topic services
The same Monitor.Enter/Exit-without-try/finally as the ClientService partials,
in the six pairs the earlier pass did not reach: it only covered ClientService.*,
and these two services are separate files owned by it. Four in ForumTopicService,
two in DirectMessagesChatTopicService, and the freeze mode is the same — a throw
between a pair leaks _order for good, and the topic list for that chat never
sorts again.
LoadForumTopicsAsync was the interesting one. It called tsc.SetResult inside the
monitor, so the continuation waiting on it — GetForumTopicsAsyncImpl, which takes
_order itself — ran inline while the lock was held. Monitor being recursive is
the only reason that was a surprise rather than a deadlock, and a throw in there
would have skipped the Exit. It now builds the result under the lock and
completes the task after releasing it.
The two paging methods got the same restructure as the three before them: decide
under the lock how much is still to load, then either answer from the cache
inside it or fall out and await with nothing held. That is five copies of this
method carrying the same bug, which is a better argument for merging them than
the duplicated line count ever was.
UpdateTopicOrder keeps publishing outside the lock, where the hand-placed
Monitor.Exit already put it. The aggregator publish still inside the lock in
LoadForumTopicsAsync is a separate finding and is left alone here.
Also records the review of both services in clientservice-review.md: seventeen
findings, of which these six are the first fixed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Close the lock leak in the ClientService critical sections
Thirteen Monitor.Enter/Monitor.Exit pairs had no try/finally, so anything that
threw in between leaked the lock for good. The worst is UpdateChatLastMessage,
which constructs a MessageAlbumLastMessageService while holding the Chat: one
throw there locks that chat forever, the next UpdateChatPosition for it blocks
the TDLib receive thread, and since that thread is the only one draining
td_receive, every update in the app stops arriving with no crash to show for it.
GetChatFolders takes lock (chat) from the UI thread, so the chat list wedges too.
All thirteen are plain lock blocks now. The ten synchronous ones are a straight
wrap. The three paging methods could not be, because await is illegal inside
lock — which is exactly why they were written with a hand-placed Monitor.Exit
before the await in the first place. Each now decides under the lock how much is
still to be loaded and either builds its result and returns inside the lock, or
falls out of it and awaits with nothing held. Same semantics, same lock ordering,
one exit path instead of three.
The MOCKUP blocks in those three methods moved but were left as they are. MOCKUP
is not defined in any configuration and the blocks reference an undefined index
variable, so they have not compiled in a long time; quietly repairing dead code
that cannot be tested does not belong in a lock-safety change.
UpdateChatDraftMessage was not in the review that prompted this — it turned up
grepping for Monitor. after converting the twelve that were listed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 533cbd9 • 3 files, +335/-147
Close the lock leak in the two topic services
The same Monitor.Enter/Exit-without-try/finally as the ClientService partials,
in the six pairs the earlier pass did not reach: it only covered ClientService.*,
and these two services are separate files owned by it. Four in ForumTopicService,
two in DirectMessagesChatTopicService, and the freeze mode is the same — a throw
between a pair leaks _order for good, and the topic list for that chat never
sorts again.
LoadForumTopicsAsync was the interesting one. It called tsc.SetResult inside the
monitor, so the continuation waiting on it — GetForumTopicsAsyncImpl, which takes
_order itself — ran inline while the lock was held. Monitor being recursive is
the only reason that was a surprise rather than a deadlock, and a throw in there
would have skipped the Exit. It now builds the result under the lock and
completes the task after releasing it.
The two paging methods got the same restructure as the three before them: decide
under the lock how much is still to load, then either answer from the cache
inside it or fall out and await with nothing held. That is five copies of this
method carrying the same bug, which is a better argument for merging them than
the duplicated line count ever was.
UpdateTopicOrder keeps publishing outside the lock, where the hand-placed
Monitor.Exit already put it. The aggregator publish still inside the lock in
LoadForumTopicsAsync is a separate finding and is left alone here.
Also records the review of both services in clientservice-review.md: seventeen
findings, of which these six are the first fixed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/develop • c603834 • 3 files, +104/-74
Close the lock leak in the group call participants and chat positions
An app-wide grep for Monitor.Enter after the topic services turned up four more
files with the same unprotected pairs. Two of them are worth fixing now.
TdExtensions.GetPosition and GetOrder are the ones that matter. Each takes an
early Monitor.Exit inside a loop and another after it, and what they lock is the
Chat itself — the same objects ClientService.OnResult locks on the receive
thread. A throw inside AreTheSame on the UI thread would have leaked that chat's
monitor and stopped update delivery for the whole app, which is the same failure
the ClientService pass was closing. return inside lock releases correctly, so
both lose their early exits entirely.
VoipGroupCallParticipants turns out to be a sixth copy of the paging method,
with the same two shapes as the five already converted, so it gets the same
treatment: decide under the lock how many participants are still to load, then
answer from the cache inside it or fall out and await with nothing held. Its
three return paths are unchanged, including the null for a response that is
neither Ok nor Error.
That the count of this method went from three to five to six as the search
widened is a better argument for merging them than the duplicated line count.
DiceView and VideoNoteContent have the same bug and are left for now: both are
in UI code rather than on the receive thread, so they can wedge a control but
not the update pipeline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • d9a6e67 • 2 files, +15/-2
Ignore a second VoipManager::Start
Assigning over m_impl would destroy the running instance without tgcalls' own
teardown. Nothing calls Start twice today, but it was the last hole left in the
Start/Stop lifecycle after the destructors went in.
Keeping the instance in flight is the right way round: it is the one serving the
live call, and a second descriptor would be for that same call.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 3321c86 • 2 files, +46/-6
Sort the offered call protocols numerically
The versions were compared as strings, so the registered set
2.7.7 5.0.0 7.0.0 8.0.0 9.0.0 10.0.0 11.0.0
came out as 9 8 7 5 2.7.7 11 10 — the two newest protocols at the end of a list
the server reads newest first. Filed during the review as latent, on the
assumption the majors were single digits. They have not been for a while.
Compared component by component as numbers now, checked against that set and
for the strict weak ordering std::sort requires.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Close the lock leak in the group call participants and chat positions
An app-wide grep for Monitor.Enter after the topic services turned up four more
files with the same unprotected pairs. Two of them are worth fixing now.
TdExtensions.GetPosition and GetOrder are the ones that matter. Each takes an
early Monitor.Exit inside a loop and another after it, and what they lock is the
Chat itself — the same objects ClientService.OnResult locks on the receive
thread. A throw inside AreTheSame on the UI thread would have leaked that chat's
monitor and stopped update delivery for the whole app, which is the same failure
the ClientService pass was closing. return inside lock releases correctly, so
both lose their early exits entirely.
VoipGroupCallParticipants turns out to be a sixth copy of the paging method,
with the same two shapes as the five already converted, so it gets the same
treatment: decide under the lock how many participants are still to load, then
answer from the cache inside it or fall out and await with nothing held. Its
three return paths are unchanged, including the null for a response that is
neither Ok nor Error.
That the count of this method went from three to five to six as the search
widened is a better argument for merging them than the duplicated line count.
DiceView and VideoNoteContent have the same bug and are left for now: both are
in UI code rather than on the receive thread, so they can wedge a control but
not the update pipeline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • d9a6e67 • 2 files, +15/-2
Ignore a second VoipManager::Start
Assigning over m_impl would destroy the running instance without tgcalls' own
teardown. Nothing calls Start twice today, but it was the last hole left in the
Start/Stop lifecycle after the destructors went in.
Keeping the instance in flight is the right way round: it is the one serving the
live call, and a second descriptor would be for that same call.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 3321c86 • 2 files, +46/-6
Sort the offered call protocols numerically
The versions were compared as strings, so the registered set
2.7.7 5.0.0 7.0.0 8.0.0 9.0.0 10.0.0 11.0.0
came out as 9 8 7 5 2.7.7 11 10 — the two newest protocols at the end of a list
the server reads newest first. Filed during the review as latent, on the
assumption the majors were single digits. They have not been for a while.
Compared component by component as numbers now, checked against that set and
for the strict weak ordering std::sort requires.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/develop • 5b731d1 • 1 files, +48/-17
Reverse the ConcurrentDictionary recommendation in the review
The review ranked swapping ReaderWriterDictionary for ConcurrentDictionary as
the largest measurable win in ClientService. That was reasoning about the cost
of one lookup without ever asking how many lookups happen, and counting them
turns the recommendation around.
A full ChatCell refresh reaches a ReaderWriterDictionary eight to twelve times.
The rate of those refreshes is bounded by UI control realization — a 64px row
under a hard flick, plus aggregator refreshes of the dozen visible cells — which
puts the whole app somewhere around 0.5k to 6k lookups a second. At the ~30ns a
lock-free read would save, that is under two tenths of a millisecond per second,
or a few hundredths of one percent of a core. Wrong by a factor of a hundred it
still would not reach two percent, and at one operation per 200 microseconds the
cross-core contention that motivated the idea never happens either.
ConcurrentDictionary would cost a node allocation per entry to buy that back,
which is the wrong side of the trade in this repo. The custom class stays.
What survives is that ReaderWriterDictionary.Find allocates a closure and a LINQ
enumerator on every call to do what a foreach does for free, which is worth
fixing on its own and needs no type change.
Two things the count corrected on the way: GetChatActions is already a
ConcurrentDictionary, and GetChatFolders uses a plain Dictionary under its own
lock. Neither was ever in scope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 4efdfba • 11 files, +85/-130
Pass call payloads as arrays instead of IVector<byte>
Every byte crossing this boundary ends up base64 encoded into TDLib JSON, so an
IVector was the wrong shape at both ends: native built a COM collection, the
projection built an RCW, and the managed side called ToArray to get back the
byte[] the generated TDLib request wanted anyway.
UInt8[] instead. The projection copies once in each direction and managed hands
the array straight to TDLib. On the E2E path, which runs per frame, that removes
two COM objects, two RCWs and one of three copies.
Signaling stops being an event. It only ever had one listener, so a delegate
does the same job without an args runtime class per packet, and
SignalingDataEmittedEventArgs goes away entirely.
The frame transform also stops waiting on a shared semaphore. tgcalls builds one
frame transformer per simulcast layer and one per incoming channel, each on its
own thread, so several transforms run at once as soon as a call has two people
in it — a second Release before the first Wait would have thrown
SemaphoreFullException, and a waiter could be woken by another frame is answer.
Holding m_lock across the delegate used to serialise them and hide this; it
stopped doing that when that lock came off the callback path. Each transform now
waits on its own signal, with a timeout so a silent TDLib drops the frame rather
than wedging a media thread.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Reverse the ConcurrentDictionary recommendation in the review
The review ranked swapping ReaderWriterDictionary for ConcurrentDictionary as
the largest measurable win in ClientService. That was reasoning about the cost
of one lookup without ever asking how many lookups happen, and counting them
turns the recommendation around.
A full ChatCell refresh reaches a ReaderWriterDictionary eight to twelve times.
The rate of those refreshes is bounded by UI control realization — a 64px row
under a hard flick, plus aggregator refreshes of the dozen visible cells — which
puts the whole app somewhere around 0.5k to 6k lookups a second. At the ~30ns a
lock-free read would save, that is under two tenths of a millisecond per second,
or a few hundredths of one percent of a core. Wrong by a factor of a hundred it
still would not reach two percent, and at one operation per 200 microseconds the
cross-core contention that motivated the idea never happens either.
ConcurrentDictionary would cost a node allocation per entry to buy that back,
which is the wrong side of the trade in this repo. The custom class stays.
What survives is that ReaderWriterDictionary.Find allocates a closure and a LINQ
enumerator on every call to do what a foreach does for free, which is worth
fixing on its own and needs no type change.
Two things the count corrected on the way: GetChatActions is already a
ConcurrentDictionary, and GetChatFolders uses a plain Dictionary under its own
lock. Neither was ever in scope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 4efdfba • 11 files, +85/-130
Pass call payloads as arrays instead of IVector<byte>
Every byte crossing this boundary ends up base64 encoded into TDLib JSON, so an
IVector was the wrong shape at both ends: native built a COM collection, the
projection built an RCW, and the managed side called ToArray to get back the
byte[] the generated TDLib request wanted anyway.
UInt8[] instead. The projection copies once in each direction and managed hands
the array straight to TDLib. On the E2E path, which runs per frame, that removes
two COM objects, two RCWs and one of three copies.
Signaling stops being an event. It only ever had one listener, so a delegate
does the same job without an args runtime class per packet, and
SignalingDataEmittedEventArgs goes away entirely.
The frame transform also stops waiting on a shared semaphore. tgcalls builds one
frame transformer per simulcast layer and one per incoming channel, each on its
own thread, so several transforms run at once as soon as a call has two people
in it — a second Release before the first Wait would have thrown
SemaphoreFullException, and a waiter could be woken by another frame is answer.
Holding m_lock across the delegate used to serialise them and hide this; it
stopped doing that when that lock came off the callback path. Each transform now
waits on its own signal, with a timeout so a silent TDLib drops the frame rather
than wedging a media thread.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
UnigramDev/Unigram/develop • 8fd02b8 • 4 files, +248/-83
Guard the ForumTopicService collections, and finish Clear()
Two findings from the same review, landing together because the review doc
records both and the commit hook commits whole files, so splitting them would
mean checking one item off before its fix existed.
ForumTopicService kept six of its eight collections in plain Dictionary, List
and HashSet: _topics, _messages, _pinnedTopicIds, _deletedTopicIds,
_pendingNewTopics and _pendingLastReadInboxMessageId. Every Update* method
writes them from the TDLib thread while GetTopic and GetTopics read them from
the UI thread — and GetTopic writes too, since recording a pending request is
what stops a miss from sending one getForumTopic per enumeration. Only _order
and _unreadTopicIds were guarded. A concurrent mutation of a Dictionary does not
throw, it spins.
All eight now sit behind one private lock, absorbing the two existing lock
objects so there is a single domain and no ordering between them to get wrong.
A ReaderWriterDictionary would have matched DirectMessagesChatTopicService, but
it covers only the two Dictionary fields; the List, the SortedSet and the three
HashSets would still need a lock, leaving two domains and real compounds
spanning both — UpdatePinnedTopics reads _pinnedTopicIds then _topics, Order
reads _deletedTopicIds and _pinnedTopicIds, the batch load touches four at once.
One lock makes those atomic and a lock cycle impossible. The dictionary reads go
through TryGetTopic/TryGetTopicByMessage so the backing store stays cheap to
change if that is revisited.
Critical sections stay small and publishes stay outside them, which shrinks the
separate finding about publishing under the lock rather than growing it:
SetPinnedForumTopics and UpdatePinnedTopics now collect under the lock and
reorder outside it, leaving only the batch load still publishing while held. The
ForumTopic objects themselves are still handed to the UI and mutated by the
update methods unsynchronised, exactly as ClientService does with Chat and User;
this fixes container corruption, which is the part that spins forever.
Clear() was missing eight caches. _activeStories was the one that bit: the story
ordering was cleared but the stories were not, so after logout GetActiveStories
still served the previous account's state. The three download sets moved into a
ClearDownloads helper next to their declarations, since they need _downloadsLock
and the reason they must not outlive an authorization belongs where they are
declared — file ids only mean anything within one session, and those sets are
also the only state here that grows for the life of the process.
Checked by enumerating every private field across the seven partials and diffing
against what Clear() touches, rather than by re-reading it: what it now leaves
alone is the injected dependencies and the lock objects, and nothing else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 330db01 • 2 files, +17/-5
Fix monospace font lookup
UnigramDev/Unigram/develop • 26179e5 • 2 files, +12/-6
Resolve only the unknown sources on the second pass
When any ssrc could not be matched, the retry walked the whole request again and
re-added every source the first pass had already resolved. It stayed harmless
only because tgcalls asks for one ssrc at a time, which is exactly the assumption
the comment above it leans on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
Guard the ForumTopicService collections, and finish Clear()
Two findings from the same review, landing together because the review doc
records both and the commit hook commits whole files, so splitting them would
mean checking one item off before its fix existed.
ForumTopicService kept six of its eight collections in plain Dictionary, List
and HashSet: _topics, _messages, _pinnedTopicIds, _deletedTopicIds,
_pendingNewTopics and _pendingLastReadInboxMessageId. Every Update* method
writes them from the TDLib thread while GetTopic and GetTopics read them from
the UI thread — and GetTopic writes too, since recording a pending request is
what stops a miss from sending one getForumTopic per enumeration. Only _order
and _unreadTopicIds were guarded. A concurrent mutation of a Dictionary does not
throw, it spins.
All eight now sit behind one private lock, absorbing the two existing lock
objects so there is a single domain and no ordering between them to get wrong.
A ReaderWriterDictionary would have matched DirectMessagesChatTopicService, but
it covers only the two Dictionary fields; the List, the SortedSet and the three
HashSets would still need a lock, leaving two domains and real compounds
spanning both — UpdatePinnedTopics reads _pinnedTopicIds then _topics, Order
reads _deletedTopicIds and _pinnedTopicIds, the batch load touches four at once.
One lock makes those atomic and a lock cycle impossible. The dictionary reads go
through TryGetTopic/TryGetTopicByMessage so the backing store stays cheap to
change if that is revisited.
Critical sections stay small and publishes stay outside them, which shrinks the
separate finding about publishing under the lock rather than growing it:
SetPinnedForumTopics and UpdatePinnedTopics now collect under the lock and
reorder outside it, leaving only the batch load still publishing while held. The
ForumTopic objects themselves are still handed to the UI and mutated by the
update methods unsynchronised, exactly as ClientService does with Chat and User;
this fixes container corruption, which is the part that spins forever.
Clear() was missing eight caches. _activeStories was the one that bit: the story
ordering was cleared but the stories were not, so after logout GetActiveStories
still served the previous account's state. The three download sets moved into a
ClearDownloads helper next to their declarations, since they need _downloadsLock
and the reason they must not outlive an authorization belongs where they are
declared — file ids only mean anything within one session, and those sets are
also the only state here that grows for the life of the process.
Checked by enumerating every private field across the seven partials and diffing
against what Clear() touches, rather than by re-reading it: what it now leaves
alone is the injected dependencies and the lock objects, and nothing else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UnigramDev/Unigram/develop • 330db01 • 2 files, +17/-5
Fix monospace font lookup
UnigramDev/Unigram/develop • 26179e5 • 2 files, +12/-6
Resolve only the unknown sources on the second pass
When any ssrc could not be matched, the retry walked the whole request again and
re-added every source the first pass had already resolved. It stayed harmless
only because tgcalls asks for one ssrc at a time, which is exactly the assumption
the comment above it leans on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#unigram
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