From 1dc02e6dbc31a7b83c4f640f38a9e4634b06814b Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 19 Aug 2019 15:16:00 -0700 Subject: [PATCH] fix: fall back to default logs path in getPath('logs') (#19653) --- docs/api/app.md | 4 ++- native_mate/native_mate/arguments.h | 12 ++++++++ shell/browser/api/atom_api_app.cc | 29 +++++++++++-------- shell/browser/api/atom_api_app.h | 3 +- shell/browser/api/atom_api_app_mac.mm | 10 +++---- .../native_mate_converters/value_converter.h | 25 ++++++++++++++++ spec-main/api-app-spec.ts | 25 ++++++++++++++++ 7 files changed, 89 insertions(+), 19 deletions(-) diff --git a/docs/api/app.md b/docs/api/app.md index a4846d004f2b..44950949e3a1 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -582,7 +582,7 @@ them. Sets or creates a directory your app's logs which can then be manipulated with `app.getPath()` or `app.setPath(pathName, newPath)`. -Calling `app.setAppLogsPath()` without a `path` parameter will result in this directory being set to `/Library/Logs/YourAppName` on _macOS_, and inside the `userData` directory on _Linux_ and _Windows_. +Calling `app.setAppLogsPath()` without a `path` parameter will result in this directory being set to `~/Library/Logs/YourAppName` on _macOS_, and inside the `userData` directory on _Linux_ and _Windows_. ### `app.getAppPath()` @@ -614,6 +614,8 @@ Returns `String` - The current application directory. Returns `String` - A path to a special directory or file associated with `name`. On failure, an `Error` is thrown. +If `app.getPath('logs')` is called without called `app.setAppLogsPath()` being called first, a default log directory will be created equivalent to calling `app.setAppLogsPath()` without a `path` parameter. + ### `app.getFileIcon(path[, options])` * `path` String diff --git a/native_mate/native_mate/arguments.h b/native_mate/native_mate/arguments.h index 956c998d95ca..3a8a07fdf315 100644 --- a/native_mate/native_mate/arguments.h +++ b/native_mate/native_mate/arguments.h @@ -9,6 +9,7 @@ #include #include "base/macros.h" +#include "base/optional.h" #include "native_mate/converter.h" namespace mate { @@ -34,6 +35,17 @@ class Arguments { return ConvertFromV8(isolate_, info_->Data(), out); } + template + bool GetNext(base::Optional* out) { + if (next_ >= info_->Length()) + return true; + v8::Local val = (*info_)[next_]; + bool success = ConvertFromV8(isolate_, val, out); + if (success) + next_++; + return success; + } + template bool GetNext(T* out) { if (next_ >= info_->Length()) { diff --git a/shell/browser/api/atom_api_app.cc b/shell/browser/api/atom_api_app.cc index bf8a7e71ce62..062631e89b6a 100644 --- a/shell/browser/api/atom_api_app.cc +++ b/shell/browser/api/atom_api_app.cc @@ -11,6 +11,7 @@ #include "base/environment.h" #include "base/files/file_path.h" #include "base/files/file_util.h" +#include "base/optional.h" #include "base/path_service.h" #include "base/system/sys_info.h" #include "chrome/browser/browser_process.h" @@ -845,14 +846,14 @@ void App::SetAppPath(const base::FilePath& app_path) { } #if !defined(OS_MACOSX) -void App::SetAppLogsPath(mate::Arguments* args) { - base::FilePath custom_path; - if (args->GetNext(&custom_path)) { - if (!custom_path.IsAbsolute()) { +void App::SetAppLogsPath(base::Optional custom_path, + mate::Arguments* args) { + if (custom_path.has_value()) { + if (!custom_path->IsAbsolute()) { args->ThrowError("Path must be absolute"); return; } - base::PathService::Override(DIR_APP_LOGS, custom_path); + base::PathService::Override(DIR_APP_LOGS, custom_path.value()); } else { base::FilePath path; if (base::PathService::Get(DIR_USER_DATA, &path)) { @@ -867,18 +868,22 @@ void App::SetAppLogsPath(mate::Arguments* args) { base::FilePath App::GetPath(mate::Arguments* args, const std::string& name) { bool succeed = false; base::FilePath path; + int key = GetPathConstant(name); - if (key >= 0) + if (key >= 0) { succeed = base::PathService::Get(key, &path); - if (!succeed) { - if (name == "logs") { - args->ThrowError("Failed to get '" + name + - "' path: setAppLogsPath() must be called first."); - } else { - args->ThrowError("Failed to get '" + name + "' path"); + // If users try to get the logs path before setting a logs path, + // set the path to a sensible default and then try to get it again + if (!succeed && name == "logs") { + base::ThreadRestrictions::ScopedAllowIO allow_io; + SetAppLogsPath(base::Optional(), args); + succeed = base::PathService::Get(key, &path); } } + if (!succeed) + args->ThrowError("Failed to get '" + name + "' path"); + return path; } diff --git a/shell/browser/api/atom_api_app.h b/shell/browser/api/atom_api_app.h index 34530368bfac..0376256114c1 100644 --- a/shell/browser/api/atom_api_app.h +++ b/shell/browser/api/atom_api_app.h @@ -164,7 +164,8 @@ class App : public AtomBrowserClient::Delegate, void ChildProcessLaunched(int process_type, base::ProcessHandle handle); void ChildProcessDisconnected(base::ProcessId pid); - void SetAppLogsPath(mate::Arguments* args); + void SetAppLogsPath(base::Optional custom_path, + mate::Arguments* args); // Get/Set the pre-defined path in PathService. base::FilePath GetPath(mate::Arguments* args, const std::string& name); diff --git a/shell/browser/api/atom_api_app_mac.mm b/shell/browser/api/atom_api_app_mac.mm index e74a765a5d93..420f009d6e7f 100644 --- a/shell/browser/api/atom_api_app_mac.mm +++ b/shell/browser/api/atom_api_app_mac.mm @@ -13,14 +13,14 @@ namespace electron { namespace api { -void App::SetAppLogsPath(mate::Arguments* args) { - base::FilePath custom_path; - if (args->GetNext(&custom_path)) { - if (!custom_path.IsAbsolute()) { +void App::SetAppLogsPath(base::Optional custom_path, + mate::Arguments* args) { + if (custom_path.has_value()) { + if (!custom_path->IsAbsolute()) { args->ThrowError("Path must be absolute"); return; } - base::PathService::Override(DIR_APP_LOGS, custom_path); + base::PathService::Override(DIR_APP_LOGS, custom_path.value()); } else { NSString* bundle_name = [[[NSBundle mainBundle] infoDictionary] objectForKey:@"CFBundleName"]; diff --git a/shell/common/native_mate_converters/value_converter.h b/shell/common/native_mate_converters/value_converter.h index 6be0c5cde36d..5d0495808a0a 100644 --- a/shell/common/native_mate_converters/value_converter.h +++ b/shell/common/native_mate_converters/value_converter.h @@ -7,6 +7,8 @@ #include "native_mate/converter.h" +#include "base/optional.h" + namespace base { class DictionaryValue; class ListValue; @@ -33,6 +35,29 @@ struct Converter { const base::Value& val); }; +template +struct Converter> { + static bool FromV8(v8::Isolate* isolate, + v8::Local val, + base::Optional* out) { + if (val->IsNull() || val->IsUndefined()) { + return true; + } + T converted; + if (Converter::FromV8(isolate, val, &converted)) { + return true; + } + out->emplace(converted); + return true; + } + static v8::Local ToV8(v8::Isolate* isolate, + const base::Optional& val) { + if (val.has_value()) + return Converter::ToV8(val.value()); + return v8::Undefined(isolate); + } +}; + template <> struct Converter { static bool FromV8(v8::Isolate* isolate, diff --git a/spec-main/api-app-spec.ts b/spec-main/api-app-spec.ts index 97735adc20b9..7947572bea61 100644 --- a/spec-main/api-app-spec.ts +++ b/spec-main/api-app-spec.ts @@ -5,6 +5,7 @@ import * as https from 'https' import * as net from 'net' import * as fs from 'fs' import * as path from 'path' +import { homedir } from 'os' import split = require('split') import { app, BrowserWindow, Menu } from 'electron' import { emittedOnce } from './events-helpers'; @@ -671,6 +672,30 @@ describe('app module', () => { }) }) + describe('getPath("logs")', () => { + const logsPaths = { + 'darwin': path.resolve(homedir(), 'Library', 'Logs'), + 'linux': path.resolve(homedir(), 'AppData', app.name), + 'win32': path.resolve(homedir(), 'AppData', app.name), + } + + it('has no logs directory by default', () => { + // this won't be deterministic except on CI since + // users may or may not have this dir + if (!isCI) return + + const osLogPath = (logsPaths as any)[process.platform] + expect(fs.existsSync(osLogPath)).to.be.false + }) + + it('creates a new logs directory if one does not exist', () => { + expect(() => { app.getPath('logs') }).to.not.throw() + + const osLogPath = (logsPaths as any)[process.platform] + expect(fs.existsSync(osLogPath)).to.be.true + }) + }) + describe('getPath(name)', () => { it('returns paths that exist', () => { const paths = [