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

add ability to access logs in getPath() #10191

Merged
merged 17 commits into from Sep 29, 2017
Merged

add ability to access logs in getPath() #10191

merged 17 commits into from Sep 29, 2017

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Aug 3, 2017

Adds the ability to get logs via app.getPath('logs'), implementing feature requested in #10118.

  • MacOS
  • Linux
  • Windows

Nota Bene: Unlike MacOS, Linux & Windows do not create log folders until a log-worthy event happens, so this implementation lets the app developer ensure the directory exists. If anyone has an idea for an alternate way this could be dealt with please let me know!

@jkleinsc jkleinsc changed the title add ability to access logs in getPath() [WIP] add ability to access logs in getPath() Aug 7, 2017
@@ -55,6 +58,8 @@

namespace brightray {

void OverrideMacAppLogsPath();
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment about the Mac logs file being somewhere else because of Objective-C implementation.

@@ -89,6 +89,18 @@ void OverrideLinuxAppDataPath() {
PathService::Override(DIR_APP_DATA, path);
}

// void OverrideWinAppLogsPath() {
// base::FilePath path;
// // should be in c:\program files\myapp\logs ?
Copy link
Member

Choose a reason for hiding this comment

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

The logs location should be in %LOCALAPPDATA% or %APPDATA so we don't need admin permissions to write the file

Copy link
Member Author

Choose a reason for hiding this comment

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

what commit are you looking at? that was replaced a while ago haha

Copy link
Member

Choose a reason for hiding this comment

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

Wow... I just clicked the link in my notifications and went to the diff 😕

weird 😆

Copy link
Member Author

@codebytere codebytere Aug 9, 2017

Choose a reason for hiding this comment

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

should be

void OverrideWinAppLogsPath() {
  std::string appName = GetApplicationName();
  std::string logPath = "%HOMEDRIVE%%HOMEPATH%\\AppData\\Roaming\\";
  std::string appLogPath = logPath + appName + "\\logs";

  int status = mkdir(appLogPath.c_str(), S_IRWXU | S_IRGRP | S_IROTH);

  PathService::Override(DIR_APP_LOGS, base::FilePath(appLogPath));
}

in the current tree :D

@zeke
Copy link
Contributor

zeke commented Aug 10, 2017

Hey @codebytere do you wanna take a shot at adding some docs for this too?

@@ -108,7 +137,7 @@ int BrowserX11IOErrorHandler(Display* d) {
// Wait for the UI thread (which has a different connection to the X server)
// to get the error. We can't call shutdown from this thread without
// tripping an error. Doing it through a function so that we'll be able
// to see it in any crash dumps.
// to see it in any crash dumps.back
Copy link
Member

Choose a reason for hiding this comment

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

Awkward word 😄

void OverrideLinuxAppLogsPath() {
std::string appName = GetApplicationName();
std::string homePath = std:string(getenv("HOME"));
std::string appLogPath = homePath + "/.config/" + appName + "/logs";
Copy link
Member

Choose a reason for hiding this comment

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

We're gonna get users asking this to respect $XDG_CONFIG_HOME , current paths don't though (and there is an issue for it). So do what you will 😆

@zeke zeke self-assigned this Aug 16, 2017
@zeke
Copy link
Contributor

zeke commented Sep 20, 2017

Sorry for not getting to this earlier, @codebytere. Would you mind rebasing with master and pushing the changes up to see if we can get CI passing?

@zeke
Copy link
Contributor

zeke commented Sep 24, 2017

@codebytere I rebased with master (again) and am seeing this error on CircleCI:

../../brightray/browser/browser_main_parts.cc:110:29: error: unexpected ':' in nested name specifier; did you mean '::'?
  std::string homePath = std:string(getenv("HOME"));

@zeke zeke requested a review from zcbenz September 28, 2017 16:48
@zeke
Copy link
Contributor

zeke commented Sep 28, 2017

@zcbenz does this look good to you?

@codebytere codebytere changed the title [WIP] add ability to access logs in getPath() add ability to access logs in getPath() Sep 29, 2017
zcbenz
zcbenz previously requested changes Sep 29, 2017
std::string logPath = "%HOMEDRIVE%%HOMEPATH%\\AppData\\Roaming\\";
std::string appLogPath = logPath + appName + "\\logs";

status = mkdir(appLogPath.c_str(), S_IRWXU | S_IRGRP | S_IROTH);
Copy link
Member

Choose a reason for hiding this comment

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

The App::GetPath API (which calls PathService::Get) automatically creates the dir so there is no need to create it here.

@@ -55,6 +60,8 @@

namespace brightray {

void OverrideMacAppLogsPath();
Copy link
Member

Choose a reason for hiding this comment

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

The OverrideMacAppLogsPath is probably better to be a member of BrowserMainParts to make things clearer, you can reference how InitializeMainNib was done.

@@ -4,13 +4,18 @@

#include "brightray/browser/browser_main_parts.h"

#include <stdlib.h>
#include <sys/stat.h>
Copy link
Member

Choose a reason for hiding this comment

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

This header should be guarded with #if defined(OS_POSIX) since it is not available on Windows.

@@ -156,11 +163,41 @@ BrowserMainParts::BrowserMainParts() {
BrowserMainParts::~BrowserMainParts() {
}

void OverrideWinAppLogsPath() {
int status;
std::string appName = GetApplicationName();
Copy link
Member

Choose a reason for hiding this comment

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

Should use app_name to conform the coding style.

@@ -156,11 +163,41 @@ BrowserMainParts::BrowserMainParts() {
BrowserMainParts::~BrowserMainParts() {
}

void OverrideWinAppLogsPath() {
Copy link
Member

Choose a reason for hiding this comment

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

OverrideWinAppLogsPath and OverrideLinuxAppLogsPath are similar enough to be merged into one function, and you can also rename OverrideMacAppLogsPath to the same to make code simpler:

#if defined(OS_WIN) || defined(OS_LINUX)
void OverrideAppLogsPath() {
  std::string app_name = GetApplicationName();
#if defined(OS_WIN)
  ...
  std::string app_log_path = ...
#else
  ...
  std::string app_log_path = ...
#endif

  PathService::Override(DIR_APP_LOGS, base::FilePath(app_log_path));
}
#endif

void BrowserMainParts::PreEarlyInitialization() {
  ...
  OverrideAppLogsPath();
}

NSString* logsPath = [NSString stringWithFormat:@"Library/Logs/%@",bundleName];

NSString* libraryPath = [NSHomeDirectory() stringByAppendingPathComponent:logsPath];
std::string libPathString = std::string([libraryPath UTF8String]);
Copy link
Member

Choose a reason for hiding this comment

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

base::FilePath accepts StringPiece as input, so the std::string is not needed:

base::FilePath([libraryPath UTF8String])

@zeke zeke dismissed zcbenz’s stale review September 29, 2017 18:03

All requested changes were made.

zeke
zeke approved these changes Sep 29, 2017
@zeke zeke merged commit 9f89587 into master Sep 29, 2017
@zeke zeke deleted the add_log_path_support branch September 29, 2017 18:11
void BrowserMainParts::OverrideAppLogsPath() {
base::FilePath path;
NSString* bundleName = [[[NSBundle mainBundle] infoDictionary]
objectForKey:@"CFBundleName"];

Choose a reason for hiding this comment

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

(id)kCFBundleNameKey

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.

None yet

7 participants