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

Fix appname on linux #11980

Merged
merged 8 commits into from Feb 20, 2018

Conversation

Projects
None yet
3 participants
@ckerr
Copy link
Member

ckerr commented Feb 20, 2018

Rationale

Notifications can't currently be tested in CI and the 2.0.0-beta.1 window is about to close, so this PR is a subset of the linux-appname branch with the API changes in brightray/ and atom/ (the breaking changes) but without the notification code changes, which are nonbreaking fixes that can be landed later when they are testable.

Changes

  • Add new function brightray::OverrideApplicationVersion() so that version info will be in brightray's scope, and reimplement app.setVersion() to use it
  • On Linux, implement brightray::GetApplicationVersion() to use the value of app.setVersion() if set
  • On Linux, implement brightray::GetApplicationName() to use (1) the value of app.setName() if available or (2) the 'Name' entry from the app's .desktop file.
  • Fix pre-existing API spelling error: s/Overriden/Overridden/ in brightray::GetOverridenApplicationName()

@ckerr ckerr requested a review from as a code owner Feb 20, 2018

@codebytere
Copy link
Member

codebytere left a comment

lgtm

@ckerr ckerr merged commit 8d086a4 into master Feb 20, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@ckerr ckerr deleted the fix-appname-on-linux branch Feb 20, 2018

}

return name_override_;
std::string ret = name_override_;

This comment has been minimized.

@deepak1556

deepak1556 Feb 20, 2018

Member

It would be better to reuse brightray::GetOverridenApplicationName and get rid of the name_override_ variable.


GDesktopAppInfo* get_desktop_app_info() {
std::unique_ptr<base::Environment> env(base::Environment::Create());
const std::string desktop_id = libgtkui::GetDesktopName(env.get());

This comment has been minimized.

@deepak1556

deepak1556 Feb 20, 2018

Member

Doesn't it default to chromium-browser.desktop https://cs.chromium.org/chromium/src/chrome/browser/ui/libgtkui/gtk_util.cc?type=cs&q=GetDesktopname&sq=package:chromium&l=132 when the CHROME_DESKTOP env isn't set, should we avoid this default value ?

This comment has been minimized.

@ckerr

ckerr Feb 20, 2018

Author Member

I agree with both these points & will address them in the linux-appname branch

This comment has been minimized.

@ckerr

ckerr Feb 21, 2018

Author Member

Addressed in 38dd5ce and c8d2487

sethlu added a commit to sethlu/electron that referenced this pull request May 3, 2018

Fix brightray::GetApplicationName(), ..Version() on Linux (electron#1…
…1980)

* add brightray API to override app version

* in atom browser, use brightray app version API

* on Linux, have GetApplicationVersion() use brightray version API

* on Linux, implement brightray::GetApplicationName()

* fix typo in brightray API

* make browser.GetName() logic follow GetVersion() logic

* improve variable name in OverrideApplicationVersion declaration

* fix typo in brightray impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.