Status Update
Comments
lu...@gmail.com <lu...@gmail.com> #2
Ok, here is a fix:
422524: Fix vertical alignment of header icon and text |
I have spent enough time to fully understand what is going on, so I could as well fix the issue. :-) What convinced me is that even all the current installations (e.g. chromium-review) have improved. So this was not just a feature request for using other icon sizes, but this was an alignment bug everywhere, even though it mostly matters just 1 or 2 pixels.
ns...@gmail.com <ns...@gmail.com> #3
ki...@gmail.com <ki...@gmail.com> #4
na...@linaro.org <na...@linaro.org> #5
Branch: stable-3.8
commit 518840c5b86419c19e4fa2616e273a25998fad9b
Author: Ben Rohlfs <brohlfs@google.com>
Date: Fri Apr 26 11:28:12 2024
Fix vertical alignment of header icon and text
There is generally a slight misalignment of icon and text as can be
seen in this screenshot:
It was also noted in the referenced bug that increasing the icon height
is impossible, because then the alignment gets totally messed up.
3 problems contribute to this misalignment:
1. `.titleText` element being a span. This generally makes alignment of
icons next to text very tricky. Let's use a flex element instead.
2. `vertical-align: text-bottom` for the icon is a very unusual choice
for the vertical-align property and does not really make a lot of
sense. Let's just use the `align-items: center` style for flex
elements, see above.
3. `.bigTitle` has a custom font size, but not a custom line-height.
That is generally problematic as can be seen here: The element will
get a height of 20px as an inline element, but contains text with a
24.5px font. No surprise that we see misalignment. Let's make sure
that the line-height and thus the height of the text element has a
proper value. 1.2*font-size is what browsers would normally use.
This was checked to behave well on gerrit-review, android-review and
chromium-review.
Release-Notes: skip
Bug:
Change-Id: Ia09bbe92158f7b4b1d6b88e599e0635fa9b15d5e
(cherry picked from commit 06cfcb6ef54d78d7e68dfc0f17a15d30d6aae414)
M polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
M polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts
lu...@gmail.com <lu...@gmail.com> #6
@Luca, are there any config options that can be set to workaround this issue?
I believe we cannot avoid this issue, unless we redesign the overall creation of a change. We just need to make sure that Gerrit still works under this condition, which is 99% working from my experience.
We'll need to create some "child tickets" of Gerrit misbehaving when these conditions occur and fix those misbehaviours IMHO.
na...@linaro.org <na...@linaro.org> #7
I believe we cannot avoid this issue, unless we redesign the overall creation of a change.
Would it be that big of a refactor? You could probably do something like serialize change creation per-project? I would guess that fixes this issue so long as index.indexChangesAsync
is set to the default false
. There's already some serialization during change creation in order to get a unique change number from the sequence.
ap...@google.com <ap...@google.com> #8
Branch: stable-3.7
commit 4151e6f9efd71d9239bd2b65cac17790239b454c
Author: Luca Milanesio <luca.milanesio@gmail.com>
Date: Wed Feb 21 21:30:47 2024
Remove the useIndex option in SSH commands change id parsing
The Change I34dbf53524 introduced a specific flag for parsing
the change id in SSH commands differently for some use-cases
where there is a requirement of NOT using a Lucene index lookup.
When the flag was introduced in v2.13, Gerrit had a radically
different architecture:
- The Git repository contained only the code
- All review metadata was stored in ReviewDb
- The primary key for looking up changes was the change number
which did not require any Lucene lookups
At that time in v2.13, the introduction of the useIndex option
allowed to fix a specific SSH command for reindexing a change
in case it was missed during the change creation of patch-set
upload.
See
may end up with a stale or completely absent indexing entry.
Starting from v2.15 and then up to v3.0, the removal of
ReviewDb code-base was implemented without paying too much
attention on the consequences on the useIndex flag which was
ported "syntatically" to the newer releases but lost completely
its effectiveness. Also, the SSH commands did not have historically
enough test coverage, exposing the risks of a regression.
On the other side, the change finder is now clever enough
to use the index or the underlying repository depending on
the format of the change id, which makes a lot more sense
than a static decision based on the useIndex flag.
Also remove the unneeded reload of the change notes found and
loaded by the changeFinder, which was inadvertently added as part
of Change Idaeeb7d82.
TODO: Remove the flag completely after this change is
merged to master. The flag is kept unused in stable
versions for not invalidating the public signature
of the methods, which may be used by other parts of
Gerrit or other plugins.
Bug:
Release-Notes: Fix change id parsing in SSH commands
Change-Id: Iee2057f8978a40d37804c9198df0c1c70fdcb9eb
M java/com/google/gerrit/sshd/ChangeArgumentParser.java
Description
Background
In Gerrit code base we always work with the assumption that index is a best-effort way to find changes, but the only source of truth is the repository. That means that a change could be in the repository but not discoverable yet by a search. Index can be stale, that's why there is also a
StalenessChecker
that reconciles what is the index with what is known to be true, which is the repository.What steps will reproduce the problem?
Thread.sleep(60000L)
inChangeIndexer.doIndex()
git push origin HEAD:refs/for/master
remote: Processing changes: refs: 1, new: 1, done
git push origin HEAD:refs/for/master
What is the expected output?
Gerrit creates only one change with two patch-sets, due to the pushes made at 2. and 4.
Alternative outcome: Gerrit creates only one change with one patch-set, because the 2. was aborted and only 4. succeeded.
What do you see instead?
Gerrit creates two changes with the same primary key: repository/branch/change-id
Please provide any additional information below.
The problem originates from the false assumption that the change index is always up-to-date in realtime, which isn't the case at all. There is always a window, as small or as large as it can be, where the change is in the repo but Gerrit doesn't see it. Any additional push for the same Change-Id in that window will generate duplicates.
The assumption that repository/branch/change-id is unique for a change in Gerrit is inherently flawed.