search

sqweek / dialog

Simple cross-platform dialog API for go-lang
ISC License
493 stars 76 forks source link

Added folder browser (Windows only) and applied two Go conventions. #10

Closed Schobers closed 6 years ago

Schobers commented 6 years ago

I've implemented the browser dialog (Windows only). An example is provided with an explicit message that it is (for now?) only supported on Windows.

Secondly I've applied two Go conventions (errors starting with Err prefix and line comments instead of block comments).

sqweek commented 6 years ago

Looks cool, thanks for your work! I wondered for a moment if this should be a new spawning function on the File dialog, but that wouldn't really make sense in combination with Filter(), and dialog.Directory() is a nice clear entry point.

I think the gtk/osx implementations are fairly straightforward, so I'm happy to take them on and avoid the need to introduce the NotImplemented error at this stage.

I'm hesitant about the second commit. I appreciate you bringing the conventions to my attention; ideally I would have followed them to begin with. But at this point renaming Cancelled to ErrCancelled is a breaking API change, so I'd like to manage that separately with its own consideration rather than squeezing it in along with a new feature.

Schobers commented 6 years ago

Ok, no problem, I didn't consider that the change would break the API. If you want I can create another commit that only contains the block to line comment changes.

About the new feature: do you want me to create another pull request with only the new feature so you can start working on the GTK/OSX implementation and removing the flag again? Or do you want to squeeze the Windows & GTK/OSX implementation in single commit yourself?

sqweek commented 6 years ago

About the new feature: do you want me to create another pull request with only the new feature so you can start working on the GTK/OSX implementation and removing the flag again?

That would be great, thanks. Feel free to have the GTK/OSX stubs panic.

Note that you also have the option of updating this PR, by force pushing to the master branch of your fork. But a new PR is fine too, whichever way you prefer :)

Schobers commented 6 years ago

Done (I wasn't aware of how to update a PR). Note that I added the Title() method for DirectoryBuilder which was lacking in the previous commit.

sqweek commented 6 years ago

Thanks @Schobers! I've merged the directory browser in and added linux/osx support.