Support message, nameFieldLabel and showsTagField for macOS dialog by yamgent · Pull Request #8556 · electron/electron

yamgent · 2017-02-01T16:32:49Z

showSaveDialog() now supports additional attributes for setting the message, nameFieldLabel and showsTagField.

showOpenDialog() also supports message (nameFieldLabel and showsTagField doesn't seem to change the dialog box though).

  const {dialog} = require('electron');
  dialog.showSaveDialog({
    title: 'Save File',
    message: 'Select a location to save special file.',
    nameFieldLabel: 'Special Name',
    showsTagField: true
  });

kevinsawicki

Looks pretty good, left a few minor comments, thanks for implementing this.

kevinsawicki · 2017-02-07T17:40:46Z

lib/browser/api/dialog.js

+    }
+
+    if (showsTagField == null) {
+      showsTagField = false


Looks like the default OS value of this is true if unset so I think this should default to true to not break backwards-compatibility.

https://developer.apple.com/reference/appkit/nssavepanel/1525589-showstagfield?language=objc

kevinsawicki · 2017-02-07T17:41:20Z

atom/browser/ui/file_dialog_mac.mm

-  SetupDialog(dialog, title, button_label, default_path, filters);
+  SetupDialog(dialog, title, button_label, default_path, filters, message,
+          // NSOpenPanel does not support name_field_label and shows_tag_field
+          "", false);


The alignment here appear off, can you align this with the dialog argument on line 165?

kevinsawicki · 2017-02-07T17:41:42Z

atom/browser/ui/file_dialog_mac.mm

-  SetupDialog(dialog, title, button_label, default_path, filters);
+  SetupDialog(dialog, title, button_label, default_path, filters, message,
+          // NSOpenPanel does not support name_field_label and shows_tag_field
+          "", false);


Looks like the alignment here is off, can you align this with the dialog argument on line 142?

kevinsawicki · 2017-02-07T17:43:03Z

atom/browser/api/atom_api_dialog.cc

                    const file_dialog::Filters& filters,
+                    const std::string& message,
+                    const std::string& name_field_label,
+                    const bool& shows_tag_field,


For primitives like bool and int, we don't use references or const, so this should just be bool show_tags_field.

kevinsawicki · 2017-02-07T17:43:45Z

lib/browser/api/dialog.js


-    let {buttonLabel, defaultPath, filters, properties, title} = options
+    let {buttonLabel, defaultPath, filters, properties, title,
+      message} = options


This is fine to keep on a single line, we don't enforce hard line length limits in JS.

kevinsawicki · 2017-02-07T17:44:20Z

lib/browser/api/dialog.js

+    if (message == null) {
+      message = ''
+    } else if (typeof message !== 'string') {
+      throw new TypeError('Message must be a string')


Can you add an assertion for this error in spec/api-dialog-spec.js?

kevinsawicki · 2017-02-07T17:44:25Z

lib/browser/api/dialog.js

+    if (nameFieldLabel == null) {
+      nameFieldLabel = ''
+    } else if (typeof nameFieldLabel !== 'string') {
+      throw new TypeError('Name field label must be a string')


Can you add an assertion for this error in spec/api-dialog-spec.js?

yamgent · 2017-02-09T01:12:54Z

Thanks for your review! I will make the changes asap.

Support additional attributes available in macOS's NSSavePanel: message, nameFieldLabel and showsTagField

Support an additional attributes available in macOS's NSOpenPanel: message.

The default value of showsTagField in macOS's NSSavePanel is true. Therefore, in order to follow the standard behavior and not break backwards-compatibility, let's change the default value of showsTagField to true. Reference: https://developer.apple.com/reference/appkit/nssavepanel/1525589-showstagfield?language=objc

The normal convention in the codebase is to not use references or 'const' for primitives like 'bool' and 'int'.

yamgent · 2017-02-09T14:35:42Z

@kevinsawicki I rebased on top of the latest master and incorporated all of your suggested changes.

kevinsawicki · 2017-02-09T19:34:16Z

Thanks for this @yamgent, I merged #8623 into this branch which added a DialogSettings helper object so the method signatures in file_dialog.h no longer needed to change when we add new options and options you added could just be added to that struct and then used directly in file_dialog.mm.

Great job on this 👍 🚀

yamgent force-pushed the macos-open-save-panel branch from 84e21a0 to 8fa5c1d Compare February 2, 2017 12:22

kevinsawicki suggested changes Feb 7, 2017

View reviewed changes

kevinsawicki self-assigned this Feb 7, 2017

poiru mentioned this pull request Feb 7, 2017

Add support for checkbox with dialog.showMessageBox #8590

Merged

kevinsawicki mentioned this pull request Feb 8, 2017

Add DialogSettings helper #8623

Merged

yamgent added 10 commits February 9, 2017 20:10

🍎 Add additional options for Mac's save dialog

1d612a1

Support additional attributes available in macOS's NSSavePanel: message, nameFieldLabel and showsTagField

📝 Update docs for new attributes in showSaveDialog()

cc22149

🍎 Add additional options for Mac's open dialog

36209dd

Support an additional attributes available in macOS's NSOpenPanel: message.

📝 Update docs for new attributes in showOpenDialog()

8843fef

🏁 Fix dialog method calls for Windows

1bf8270

Fix wrong signature for gtk's ShowSaveDialog()

b0487b7

Add tests to check errors in parameters

7272364

Change qualifier of ShowSaveDialog() parameter

a4a71a1

The normal convention in the codebase is to not use references or 'const' for primitives like 'bool' and 'int'.

Fix code formatting

347dc83

yamgent force-pushed the macos-open-save-panel branch from 49f6b70 to 347dc83 Compare February 9, 2017 13:51

kevinsawicki added 3 commits February 9, 2017 11:25

Merge remote-tracking branch 'origin/master' into macos-open-save-panel

f7f4de3

Document new settings

35654d8

🎨

5130ad2

kevinsawicki approved these changes Feb 9, 2017

View reviewed changes

kevinsawicki merged commit 5bf60ad into electron:master Feb 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support message, nameFieldLabel and showsTagField for macOS dialog#8556

Support message, nameFieldLabel and showsTagField for macOS dialog#8556
kevinsawicki merged 13 commits intoelectron:masterfrom
yamgent:macos-open-save-panel

yamgent commented Feb 1, 2017

Uh oh!

kevinsawicki left a comment

Uh oh!

kevinsawicki Feb 7, 2017

Uh oh!

kevinsawicki Feb 7, 2017

Uh oh!

kevinsawicki Feb 7, 2017

Uh oh!

kevinsawicki Feb 7, 2017

Uh oh!

kevinsawicki Feb 7, 2017

Uh oh!

kevinsawicki Feb 7, 2017

Uh oh!

kevinsawicki Feb 7, 2017

Uh oh!

yamgent commented Feb 9, 2017

Uh oh!

yamgent commented Feb 9, 2017

Uh oh!

kevinsawicki commented Feb 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

yamgent commented Feb 1, 2017

Uh oh!

kevinsawicki left a comment

Choose a reason for hiding this comment

Uh oh!

kevinsawicki Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

kevinsawicki Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

kevinsawicki Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

kevinsawicki Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

kevinsawicki Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

kevinsawicki Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

kevinsawicki Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

yamgent commented Feb 9, 2017

Uh oh!

yamgent commented Feb 9, 2017

Uh oh!

kevinsawicki commented Feb 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants