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 BrowserWindow.prototype.setThumbnailToolTip #6762

Merged
merged 1 commit into from
Aug 8, 2016
Merged

Add BrowserWindow.prototype.setThumbnailToolTip #6762

merged 1 commit into from
Aug 8, 2016

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Aug 7, 2016

@@ -858,6 +864,7 @@ void Window::BuildPrototype(v8::Isolate* isolate,
.SetMethod("unhookWindowMessage", &Window::UnhookWindowMessage)
.SetMethod("unhookAllWindowMessages", &Window::UnhookAllWindowMessages)
.SetMethod("setThumbnailClip", &Window::SetThumbnailClip)
.SetMethod("setThumbnailTooltip", &Window::SetThumbnailTooltip)
Copy link
Contributor

Choose a reason for hiding this comment

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

The method on Tray is called setToolTip so I wonder if this should be setThumbnailToolTip for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

@kevinsawicki Was referring to capitalizing (the second T in ToolTip) "setThumbnailToolTip"

Copy link
Contributor Author

@miniak miniak Aug 8, 2016

Choose a reason for hiding this comment

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

I guess it's too late... (should go to sleep already) I haven't read his comment properly :)
Renamed to setThumbnailToolTip, even though I don't like it much, as I think tooltip is a single word.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also just noticed that the branch name is incorrect :)

Copy link
Member

Choose a reason for hiding this comment

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

@miniak As is the MSDN link at the top of the PR, but what's a link and a branch name between friends 😆

@miniak miniak changed the title Add BrowserWindow.prototype.setThumbnailTooltip Add BrowserWindow.prototype.setThumbnailToolTip Aug 8, 2016
@@ -667,6 +667,12 @@ bool Window::SetThumbnailClip(const gfx::Rect& region) {
return window->taskbar_host().SetThumbnailClip(
window_->GetAcceleratedWidget(), region);
}

bool Window::SetThumbnailToolTip(const std::string& tooltip) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can pass const base::string16& directly here, which is more efficient than converting std::string to UTF16 later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other methods also take std::string, I can create another PR to refactor all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@zcbenz zcbenz merged commit 872fbe8 into electron:master Aug 8, 2016
@miniak miniak deleted the set-thumbnail-clip branch August 8, 2016 11:34
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.

4 participants