Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed download item filename overlap #9759

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

nullhook
Copy link
Contributor

@nullhook nullhook commented Aug 16, 2021

Resolves brave/brave-browser#17313

The overlap was caused by a recent change we made on the UX of the download item view.
The duped filename was setting itself visible in all modes. There are various modes to take in to consideration however, we only care about the normal mode.

This is what upstream is doing: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/download/download_item_view.cc;l=816;bpv=0;bpt=1

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

A detailed test plan is described in the issue.

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, although wondering if there's a lint issue. 👍 when all CI passes

@nullhook nullhook force-pushed the bug/download-item-text-overlap branch from bcc708f to a62f6f3 Compare August 16, 2021 21:18
@bsclifton
Copy link
Member

@petemill does look wierd; but I ran clang-format on the above patch and it came out the same 👍

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good! Tested and it works as expected 🎉 (this is after hitting Retry with wifi disconnected)
image

Also tested: I was able to repro bug with Nightly 1.30.23 👍

@nullhook nullhook force-pushed the bug/download-item-text-overlap branch from a62f6f3 to 29bac7c Compare August 18, 2021 04:50
@nullhook nullhook requested a review from a team as a code owner August 18, 2021 04:50
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice find - we just need to make it a bit more robust with the view lifecycle

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful! Thanks for taking the time to move the logic into UpdateLabels 😄

I didn't see any artifacts (although didn't notice those before either) after patching this in / compiling / running it - and code is nice and clean

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small refactor, otherwise looks good

browser/ui/views/download/brave_download_item_view.cc Outdated Show resolved Hide resolved
@bsclifton bsclifton merged commit fc025c2 into master Sep 16, 2021
@bsclifton bsclifton deleted the bug/download-item-text-overlap branch September 16, 2021 18:31
@bsclifton bsclifton added this to the 1.31.x - Nightly milestone Sep 16, 2021
@kjozwiak
Copy link
Member

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.31.50 Chromium: 94.0.4606.50 (Official Build) nightly (64-bit)
-- | --
Revision | 0c1ac2c4842a4746c27c937c1a0453f98da1a972-refs/branch-heads/4606@{#1049}
OS | Windows 10 Version 20H2 (Build 19042.1237)
Paused/Interrupted Warning after internet restored Resumed
2 3 4

Example from https://testsafebrowsing.appspot.com via the Desktop Download Warnings section:

mainExample

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Illegible overlapping error text in download infobar
5 participants