Skip to content

Conversation

@github-actions
Copy link

@github-actions github-actions bot commented Feb 3, 2025

Automated changes by create-pull-request GitHub action

PIG208 and others added 30 commits February 6, 2025 18:30
We are pretty certain that the upstream commit
  flutter/flutter@df114fbe9
was the exact one that caused the increased RAM usage for the Android
build.

The upstream change causes the engine artifacts to contain debug
symbols, which is intentional (as it enables putting debug symbols in
the app's bundle artifact) but makes the artifacts about 8x larger.
That causes known regressions in CPU and memory use at build time:
  flutter/flutter#162675

Since the regression was expected, there is no action to be taken
upstream.

However, we haven't found an effective way to rewrite the build script in
a way that this is mitigated without needing to raise the limit. For the
investigation details, see CZO discussion:
  https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/Gradle.20out.20of.20memory

Since a previous bump to 3 GiB, the issue has been mitigated, but it
still happens some of the time: at least twice in the past day or so.
Add another 1 GiB to see if that addresses the flakes.
This is adapted lightly from an example app I made back in the first
week of the zulip-flutter project, 2022-12-23, as part of developing
the sticky_header library.

The example app was extremely helpful then for experimenting with
changes and seeing the effects visually and interactively, as well as
for print-debugging such experiments.  So let's get it into the tree.

The main reason I didn't send the example app back then is that it
was a whole stand-alone app tree under example/, complete with all
the stuff in android/ and ios/ and so on that `flutter create` spits
out for setting up a Flutter app.  That's pretty voluminous: well
over 100 different files totalling about 1.1MB on disk.  I did't
want to permanently burden the repo with all that, nor have to
maintain it all over time.

Happily, I realized today that we can skip that, and still have a
perfectly good example app, by reusing that infrastructure from the
actual Zulip app.  That way all we need is a Dart file with a `main`
function, corresponding to the old example's `lib/main.dart` which
was the only not-from-the-template code in the whole example app.

So here it is.  Other than moving the Dart file and discarding the
rest, the code I wrote back then has been updated to our current
formatting style; adjusted slightly for changes in Flutter's
Material widgets; and updated for changes I made to the
sticky_header API after that first week.
It's already a fact that the header's size in each dimension is
non-negative and finite; the framework asserts that in the `layout`
implementation (via debugAssertDoesMeetConstraints).

So that includes `headerExtent`; and then `paintedHeaderSize` is
bounded to between zero and that value.
This is the right thing, as the comment explains.
Conveniently it's also simpler.
…y; note a bug

The logic below -- particularly in the allowOverflow true case,
where it constructs a new SliverGeometry from scratch -- has been
implicitly relying on several of these facts already.  These
wouldn't be true of an arbitrary sliver child; but this sliver knows
what type of child it actually has, and they are true of that one.
So write down the specific assumptions we can take from that.

Reading through [RenderSliverList.performLayout] to see what it can
produce as the child geometry also made clear there's another case
that this method isn't currently correctly handling at all: the case
where scrollOffsetCorrection is non-null.  Filed an issue for that;
add a todo-comment for it.
Fixes zulip#1309.
Fixes zulip#725.

This is necessary when scrolling back to the origin over items that
grew taller while off screen (and beyond the 250px of near-on-screen
cached items).  For example that can happen because a message was
edited, or because new messages came in that were taller than those
previously present.
…sumptions

Because the child's hitTestExtent equals its paintExtent, this was
producing a new hitTestExtent equal to the new paintExtent.  But
that's the same behavior the SliverGeometry constructor gives us
by default.
This relies on (and expresses) an assumption in the new assertions:
that the child's layoutExtent equals paintExtent.

That assumption will be helpful in keeping this logic manageable to
understand as we add an upcoming further wrinkle.
In particular this will affect the upper sliver (with older messages)
in the message list, when we start using two slivers there in earnest.
Fixed a bug in MessageListTheme.lerp() where dmRecipientHeaderBg was using
streamMessageBgDefault instead of dmRecipientHeaderBg for interpolation.
I forgot to do this before today's v0.0.26, oops.

The process is only semi-automated at this point (see zulip#276).  It
was fresher in mind as of the last couple of releases, but I didn't
think of it today.  That's what a checklist is for; add it there.
This update followed a more boring normal process again, the same as
c9c5f3d.  Unlike 833b5fd, no manual adjustment needed; the
workaround that the adjustment in that commit was part of seems to
have succeeded in getting Weblate to handle `nb` appropriately.
The string is used at the end of the "errorFilesTooLarge" message,
which includes a list of files with its size that are too large.

Signed-off-by: Zixuan James Li <[email protected]>
The message, when used in lib/widgets/compose_box.dart, substitutes
`listMessage` with newline separated lines of filenames with size.
Update the example to match this usage.

Signed-off-by: Zixuan James Li <[email protected]>
to match the labels for the other fields (loginEmailLabel,
loginPasswordLabel, etc.)

This also updates existing translations in other languages to
match.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 and others added 29 commits February 18, 2025 20:27
This is another common type of error that is expected to be reused
more later

Signed-off-by: Zixuan James Li <[email protected]>
This can happen when `removeAccount` was called
during `doLoadPerAccount`, possibly due to the user logging out
from choose-account page while the store is getting reloaded.

However, it is currently not reachable live because of a bug in
`UpdateMachine.load`.  See zulip#1354.

Modulo the bug, `UpdateMachine` can safely ignore this error
acknowledging that the reload has failed, given that the user
can continue using the app by navigating from choose-account page.

Signed-off-by: Zixuan James Li <[email protected]>
The method loadPerAccount has two call sites, i.e. places
where we send register-queue requests:

1. _reloadPerAccount through [UpdateMachine._handlePollError]
   (e.g.: expired event queue)
2. perAccount through [PerAccountStoreWidget]
   (e.g.: loading for the first time)

Both sites already expect [AccountNotFoundException] by assuming that
the `loadPerAccount` fail is irrecoverable and is handled elsewhere.

This partly addresses zulip#890 by handling authentication errors for
register-queue.

Fixes: zulip#737

Signed-off-by: Zixuan James Li <[email protected]>
This makes it explicit at each call site that the method being called
is an action, not (for example) an API endpoint binding.  The
difference is important in particular because it affects how --
really, whether -- the caller should handle errors.

See discussion from when the similarity in appearance to API endpoint
bindings caused confusion:
  https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/.23F1317.20showErrorDialog/near/2080570
Like the previous commit, this hopefully helps avoid confusion about
the role of these methods when looking at their call sites.
Signed-off-by: Zixuan James Li <[email protected]>
As we add more tests for future migrations, Drift will start complaining
about opened databases that are not closed.  Always remember doing this
will ensure that we don't leak states to other database tests.

Signed-off-by: Zixuan James Li <[email protected]>
We have already upgraded drift; this reflects that we rely on newer
drift versions, but does not actually require changes to the lock files.

Signed-off-by: Zixuan James Li <[email protected]>
This saves us from writing more simple migration tests between
schema versions without data in the future.

---

The tests are inspired by the test template generated from
`dart run drift_dev make-migrations`.

To reproduce the outputs, go through the following steps:

Modify `build.yaml` by specifying the location of the database. This
step is needed because `make-migrations` does not accept this through
command line arguments.

```
targets:
  $default:
    builders:
      # ...
      drift_dev:
        options:
          databases:
            default: lib/model/database.dart
```

Then, run the following commands:

```
dart run drift_dev make-migrations
cp test/model/schemas/*.json drift_schemas/default/
dart run drift_dev make-migrations
```

The first `make-migrations` run generates the initial schema and test
files without looking at the versions we have in test/model/schemas.

Copying the schema files and running `make-migrations` will end up
creating `test/drift/default/migration_test.dart`, along with other
generated files.

See also:
  https://drift.simonbinder.eu/Migrations/#usage

Signed-off-by: Zixuan James Li <[email protected]>
Generating schema_versions.g.dart is crucial because we
otherwise only have access to the latest schema when running migrations.

That would mean that if we later drop or alter a table or column
mentioned in an existing migration, the meaning of the existing
migration would change. The affected existing migration would then
migrate to an unintended state that doesn't match what later
migrations expect, and it or a later migration might fail.

See also discussion:
  zulip#1248 (comment)

---

An alternative to using all these commands for generating files is
`dart run drift_dev make-migrations`, which is essentially a wrapper
for the `schema {dump,generate,steps}` subcommands.  `make-migrations`
let us manage multiple database schemas by configuring them with
`build.yaml`, and it dictates the which subdirectories the generated
files will be created at.

Because `make-migrations` does not offer the same level of
customizations to designate exactly where the output files will be,
opting out from it for now.

We can revisit this if it starts to offer features that are not
available with the subcommands, or that we find the need for managing
multiple databases.

See also:
  https://drift.simonbinder.eu/migrations/step_by_step/#manual-generation

Signed-off-by: Zixuan James Li <[email protected]>
It is a thin wrapper that does what we are already doing in our
migration code.

While not required, an advantage of this is that it causes compilation
errors for missing migration steps.
We previously missed tables that are not known to the schema.  This
becomes an issue if a new table is added at a newer schema level. When
we go back to an earlier schema, that new table remains in the database,
so subsequent attempts to upgrade to the later schema level that adds
the table will fail, because it already exists.

The test for this relies on some undocumented drift internals to
modify the schema version it sees when running the migration.
References:
  https://github.com/simolus3/drift/blob/18cede15/drift/lib/src/runtime/executor/helpers/engines.dart#L459-L495
  https://github.com/simolus3/drift/blob/18cede15/drift/lib/src/sqlite3/database.dart#L198-L211
  https://github.com/simolus3/sqlite3.dart/blob/4de46afd/sqlite3/lib/src/implementation/database.dart#L69-L85

Fixes: zulip#1172

Signed-off-by: Zixuan James Li <[email protected]>
Ever since 5827512 made the text input's controller start memoizing
`textNormalized` as part of its own state, there's nothing to gain by
keeping a copy of it on the widget state too.
And refactor the implementation to prepare for a new [ApiNarrow]
feature.

We're about to add ApiNarrowWith, which new in Zulip Server 9. This
function seems like a good place to resolve ApiNarrows into the
expected form for servers before 9 (without "with") and 9 or later
(with "with"). First we have to make its name and dartdoc more
generic, as done here.
We'll use this for some new tests, coming up.

Also use it now for one test that's only been checking the topic in
the app bar. That test now checks the channel name too, as it was
before b1767b2, which dropped that specificity when we split the
app bar's channel name and topic onto two rows.
This will later (in another PR) be useful for checking emptiness
when realmEmptyTopicDisplayName is given.

Signed-off-by: Zixuan James Li <[email protected]>
The '#channel > topic' style strings are not supposed to be translated
into different languages as they are Zulip's language of expressing the
desintation, not something bound to the English language.

See also:
  zulip#1148 (comment)

Signed-off-by: Zixuan James Li <[email protected]>
@github-actions github-actions bot force-pushed the update-translations/weblate branch from 2cd3f29 to 323e644 Compare February 24, 2025 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants