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

Allow for custom save/open commands. #1463

Merged
merged 3 commits into from
Dec 15, 2020
Merged

Allow for custom save/open commands. #1463

merged 3 commits into from
Dec 15, 2020

Conversation

jneem
Copy link
Collaborator

@jneem jneem commented Dec 13, 2020

This is a proposal for addressing #1455; it allows setting custom selectors for open and save panels.

Some comments:

  • I'm not crazy about the FileDialogOptions code duplication, but I couldn't think of another way to present a nice API. Selector isn't defined in druid-shell...
  • There's a breaking change with the SAVE_AS command. I think this is justified because the way it's currently being dispatched (and handled, in my experience) doesn't need the runtime choice between having a save path and not having one.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

This looks great. I think that duplication between druid and druid-shell is okay, and there is precedent in types like MouseEvent (and I think at least one other that is slipping my mind?) the reality is that druid works at a higher level than druid-shell and will often need to store different information.

The docs are also excellent.

I'm really not worried at all about the breakage.

I do think it might be nice if we eventually expose better API for this via EventCtx; I think that using commands for some of this stuff is maybe a bit of hack, even if that's how we end up handling it internally? But what we have works just fine for now, so I don't think this is a priority.

@@ -78,7 +78,7 @@ struct Inner<T> {
app: Application,
delegate: Option<Box<dyn AppDelegate<T>>>,
command_queue: CommandQueue,
file_dialogs: HashMap<FileDialogToken, WindowId>,
file_dialogs: HashMap<FileDialogToken, (WindowId, Selector<FileInfo>, Selector<()>)>,
Copy link
Member

Choose a reason for hiding this comment

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

I would probably create a named struct that holds these items, instead of a 3-tuple?

// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Copy link
Member

Choose a reason for hiding this comment

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

let's add a brief //! module comment?

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.

2 participants