close
The Wayback Machine - https://web.archive.org/web/20211119170443/https://github.com/brave/brave-browser/issues/17377
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

Jump to bottom

(Win/Linux) New Private window should be New private window, in hamburger menu #17377

Open
stephendonner opened this issue Aug 5, 2021 · 18 comments
Open

(Win/Linux) New Private window should be New private window, in hamburger menu #17377

stephendonner opened this issue Aug 5, 2021 · 18 comments
Assignees
@nullhook
Labels
bug needs-text-change OS/Desktop OS/Linux OS/Windows priority/P4 regression
Projects
Front End

Comments

@stephendonner
Copy link
Collaborator

@stephendonner stephendonner commented Aug 5, 2021 •
edited by bsclifton
Loading

Implementation note (from @mkarolin; updated by @bsclifton)

  • The _override.grd files can't be modified; these are auto-generated when we run npm run chromium_rebase_l10n
  • The strings in the _override.grd files are based on automatic generic substitutions that are mapped in https://github.com/brave/brave-core/blob/master/script/lib/grd_string_replacements.py
  • There is no mapping for Incognito -> private
  • One option to solve this is to add a new string into app/brave_generated_resources.grd and then replace IDS_NEW_INCOGNITO_WINDOW with our string ID in code via a chromium_src override.

Original issue description

New Private window should be New private window, in hamburger menu

Windows and Linux desktop only;macOS is taken care of in #17308

Steps to Reproduce

  • open the hamburger menu on the browser toolbar
  • look at the capitalization of the menu items

Actual result:

Reads New Private window

windows10-menu

Expected result:

New private window

Reproduces how often:

100%

Brave version (brave://version info)

Brave 1.30.3 Chromium: 92.0.4515.131 (Official Build) nightly (64-bit)
Revision 6b8d6c56ce21e38a72f7c4becb5abc1fa5134f29-refs/branch-heads/4515@{#1933}
OS Windows 10 OS Version 2009 (Build 22000.120)

/cc @nullhook
@stephendonner stephendonner mentioned this issue Aug 5, 2021
Merged
24 tasks
@rebron
Copy link
Collaborator

@rebron rebron commented Aug 6, 2021

@stephendonner Windows and Linux?

Loading

@rebron rebron added this to On Deck in Front End Aug 6, 2021
@stephendonner
Copy link
Collaborator Author

@stephendonner stephendonner commented Aug 6, 2021

@stephendonner Windows and Linux?

Yup.

Loading

@stephendonner stephendonner changed the title New Private window should be New private window, in hamburger menu (Win/Linux) New Private window should be New private window, in hamburger menu Aug 6, 2021
@SebastianLF
Copy link

@SebastianLF SebastianLF commented Aug 7, 2021

Let's give it a try.

Loading

@stephendonner
Copy link
Collaborator Author

@stephendonner stephendonner commented Aug 7, 2021

Let's give it a try.

Hi @SebastianLF by this, do you mean to say you'd like to try fixing this issue? If so, please confirm, and I'll assign to you; thanks!

Loading

@SebastianLF
Copy link

@SebastianLF SebastianLF commented Aug 8, 2021 •
edited
Loading

Let's give it a try.

Hi @SebastianLF by this, do you mean to say you'd like to try fixing this issue? If so, please confirm, and I'll assign to you; thanks!

Exactly. I will work on it tomorrow.

Loading

@SebastianLF
Copy link

@SebastianLF SebastianLF commented Aug 10, 2021

Package 'libgnome-keyring-dev' is no more maintained on my actual version of ubuntu 20.04. I'll install version 18.04 in a new partition on my disk and make it work.

Loading

@stephendonner
Copy link
Collaborator Author

@stephendonner stephendonner commented Aug 12, 2021

How's this going @SebastianLF? If for some reason you're (later) unable to make progress, please unassign it back, so others can also take a look; thanks!

Loading

@SebastianLF
Copy link

@SebastianLF SebastianLF commented Aug 12, 2021 •
edited
Loading

@stephendonner I installed ubuntu 18.04 and set up everything today. I certainly send a PR tomorrow.

Loading

@SebastianLF
Copy link

@SebastianLF SebastianLF commented Aug 13, 2021

when "npm run init", it starts to download 16gb of data, why is that big ?

Loading

@stephendonner stephendonner mentioned this issue Aug 13, 2021
Closed
@bsclifton
Copy link
Member

@bsclifton bsclifton commented Aug 13, 2021 •
edited
Loading

@SebastianLF running npm run init will download all of the source from Chromium (including history, which is the biggest). If you're able to compile locally, that's great. But you can still PR the text change without trying locally and we can try on our side with our CI (or building locally)

Loading

@SebastianLF
Copy link

@SebastianLF SebastianLF commented Aug 14, 2021

@SebastianLF running npm run init will download all of the source from Chromium (including history, which is the biggest). If you're able to compile locally, that's great. But you can still PR the text change without trying locally and we can try on our side with our CI (or building locally)

Thanks for your answer, I'll do this way. Is it in 'generated_resources_override.grd' or 'generated_resources.grd' that the string should be modified?
Sorry guys for the delay.

Loading

@bsclifton
Copy link
Member

@bsclifton bsclifton commented Aug 17, 2021 •
edited
Loading

@SebastianLF we have some info on our wiki here:
https://github.com/brave/brave-browser/wiki/Strings-and-Localization#chromium-strings

But I'm not seeing that addressed unfortunately. @mkarolin @petemill @mariospr do we have any docs for when to use app/generated_resources.grd versus app/generated_resources_override.grd?

Loading

@deepxcode
Copy link

@deepxcode deepxcode commented Aug 17, 2021

I can work on this issue and make the PR asap.

Loading

@AnudeepGunukula AnudeepGunukula mentioned this issue Aug 20, 2021
Closed
19 tasks
@AnudeepGunukula
Copy link

@AnudeepGunukula AnudeepGunukula commented Aug 20, 2021

Hi @bsclifton @stephendonner . i had made a pr that will close this issue
kindly review it and let me know the changes if needed.

Loading

@stephendonner
Copy link
Collaborator Author

@stephendonner stephendonner commented Aug 20, 2021

Folks, thanks for your willingness to help; @SebastianLF still has this assigned to him, so I'd like to give him first attempt at solving it; thanks!

Loading

@SebastianLF SebastianLF mentioned this issue Aug 23, 2021
Closed
24 tasks
@SebastianLF
Copy link

@SebastianLF SebastianLF commented Aug 23, 2021 •
edited
Loading

PR: brave/brave-core#9821
Let me know if it works. If any error, I will modify accordingly.

Loading

@stephendonner
Copy link
Collaborator Author

@stephendonner stephendonner commented Sep 1, 2021

PR: brave/brave-core#9821
Let me know if it works. If any error, I will modify accordingly.

Sorry for the delay; I've put a couple more reviewers on your PR, who will hopefully be able to take a look soon! 🙏

Loading

@bsclifton
Copy link
Member

@bsclifton bsclifton commented Sep 2, 2021 •
edited
Loading

Thanks for the attempt @SebastianLF - I removed the good first issue tag as there is some complexity here due to the replacement done for Chromium strings by Brave. More information available in brave/brave-core#9821 (comment)

Updated the top issue with a note

Loading

@rebron rebron moved this from On Deck to In progress in Front End Sep 14, 2021
@rebron rebron moved this from In progress to Pending review in Front End Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nullhook nullhook

Labels
bug needs-text-change OS/Desktop OS/Linux OS/Windows priority/P4 regression
Projects
Front End
  
Pending review
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

modified New Private window to New private window
7 participants
@stephendonner @bsclifton @rebron @SebastianLF @nullhook @deepxcode @AnudeepGunukula