Skip to content

Commit

Permalink
feat: change DesktopCapturerSource.display_id to displayId
Browse files Browse the repository at this point in the history
This is an API smell that's annoyed me for a while, this is a backwards-compatible
migration from display_id to displayId.  Accessing the old property will emit a deprecation
warning, it's also flagged deprecated in typescript though I'm open to changing that to be a full removal.
  • Loading branch information
MarshallOfSound committed Sep 20, 2022
1 parent 71ba841 commit 36b6c22
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 10 deletions.
3 changes: 2 additions & 1 deletion docs/api/structures/desktop-capturer-source.md
Expand Up @@ -15,11 +15,12 @@
`thumbnailSize` specified in the `options` passed to
`desktopCapturer.getSources`. The actual size depends on the scale of the
screen or window.
* `display_id` string - A unique identifier that will correspond to the `id` of
* `displayId` string - A unique identifier that will correspond to the `id` of
the matching [Display](display.md) returned by the [Screen API](../screen.md).
On some platforms, this is equivalent to the `XX` portion of the `id` field
above and on others it will differ. It will be an empty string if not
available.
* `display_id` string _Deprecated_ - Deprecated accessor for `displayId`.
* `appIcon` [NativeImage](../native-image.md) - An icon image of the
application that owns the window or null if the source has a type screen.
The size of the icon is not known in advance and depends on what
Expand Down
6 changes: 5 additions & 1 deletion shell/browser/api/electron_api_desktop_capturer.cc
Expand Up @@ -44,7 +44,11 @@ struct Converter<electron::api::DesktopCapturer::Source> {
dict.Set("thumbnail",
electron::api::NativeImage::Create(
isolate, gfx::Image(source.media_list_source.thumbnail)));
dict.Set("display_id", source.display_id);
dict.Set("displayId", source.display_id);
dict.SetDeprecated(
"display_id", source.display_id,
"The display_id property on the DesktopCapturerSource object is "
"deprecated, please us the displayId property instead");
if (source.fetch_icon) {
dict.Set(
"appIcon",
Expand Down
32 changes: 32 additions & 0 deletions shell/common/gin_helper/dictionary.h
Expand Up @@ -9,13 +9,29 @@
#include <utility>

#include "gin/dictionary.h"
#include "shell/common/gin_converters/callback_converter.h"
#include "shell/common/gin_converters/std_converter.h"
#include "shell/common/gin_helper/accessor.h"
#include "shell/common/gin_helper/function_template.h"
#include "shell/common/node_includes.h"
#include "shell/common/process_util.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace gin_helper {

namespace {

template <typename V>
v8::Local<v8::Value> DeprecatedValueProvider(const std::string& message,
v8::Isolate* isolate,
const V& value) {
electron::EmitWarning(node::Environment::GetCurrent(isolate), message,
"electron");
return gin::ConvertToV8(isolate, value);
}

} // namespace

// Adds a few more extends methods to gin::Dictionary.
//
// Note that as the destructor of gin::Dictionary is not virtual, and we want to
Expand Down Expand Up @@ -110,6 +126,22 @@ class Dictionary : public gin::Dictionary {
.ToChecked();
}

template <typename K, typename V>
bool SetDeprecated(const K& key,
const V& val,
const std::string& message,
v8::PropertyAttribute attribute = v8::None) {
auto context = isolate()->GetCurrentContext();

v8::Local<v8::Value> getter = gin::ConvertToV8(
isolate(), base::BindRepeating(&DeprecatedValueProvider<V>, message,
isolate(), val));
v8::PropertyDescriptor prop_desc(getter, v8::Undefined(isolate()));
v8::Maybe<bool> maybe = GetHandle()->DefineProperty(
context, gin::StringToV8(isolate(), key), prop_desc);
return maybe.IsJust() && maybe.FromJust();
}

template <typename K, typename V>
bool SetGetter(const K& key,
const V& val,
Expand Down
8 changes: 4 additions & 4 deletions spec/api-desktop-capturer-spec.ts
Expand Up @@ -50,25 +50,25 @@ ifdescribe(!process.arch.includes('arm') && process.platform !== 'win32')('deskt
});

// Linux doesn't return any window sources.
ifit(process.platform !== 'linux')('returns an empty display_id for window sources', async () => {
ifit(process.platform !== 'linux')('returns an empty displayId for window sources', async () => {
const w = new BrowserWindow({ width: 200, height: 200 });
await w.loadURL('about:blank');

const sources = await desktopCapturer.getSources({ types: ['window'] });
w.destroy();
expect(sources).to.be.an('array').that.is.not.empty();
for (const { display_id: displayId } of sources) {
for (const { displayId } of sources) {
expect(displayId).to.be.a('string').and.be.empty();
}
});

ifit(process.platform !== 'linux')('returns display_ids matching the Screen API', async () => {
ifit(process.platform !== 'linux')('returns displayIds matching the Screen API', async () => {
const displays = screen.getAllDisplays();
const sources = await desktopCapturer.getSources({ types: ['screen'] });
expect(sources).to.be.an('array').of.length(displays.length);

for (let i = 0; i < sources.length; i++) {
expect(sources[i].display_id).to.equal(displays[i].id.toString());
expect(sources[i].displayId).to.equal(displays[i].id.toString());
}
});

Expand Down
6 changes: 3 additions & 3 deletions spec/screen-helpers.ts
Expand Up @@ -19,14 +19,14 @@ export const captureScreen = async (point: Electron.Point = { x: 0, y: 0 }): Pro
const DEBUG_CAPTURE = false;
if (DEBUG_CAPTURE) {
for (const source of sources) {
await fs.promises.writeFile(path.join(fixtures, `screenshot_${source.display_id}.png`), source.thumbnail.toPNG());
await fs.promises.writeFile(path.join(fixtures, `screenshot_${source.displayId}.png`), source.thumbnail.toPNG());
}
}
const screenCapture = sources.find(source => source.display_id === `${display.id}`);
const screenCapture = sources.find(source => source.displayId === `${display.id}`);
// Fails when HDR is enabled on Windows.
// https://bugs.chromium.org/p/chromium/issues/detail?id=1247730
if (!screenCapture) {
const displayIds = sources.map(source => source.display_id);
const displayIds = sources.map(source => source.displayId);
throw new Error(`Unable to find screen capture for display '${display.id}'\n\tAvailable displays: ${displayIds.join(', ')}`);
}
return screenCapture.thumbnail;
Expand Down
2 changes: 1 addition & 1 deletion typings/internal-electron.d.ts
Expand Up @@ -204,7 +204,7 @@ declare namespace ElectronInternal {
id: string;
name: string;
thumbnail: Electron.NativeImage;
display_id: string;
displayId: string;
appIcon: Electron.NativeImage | null;
}

Expand Down

0 comments on commit 36b6c22

Please sign in to comment.