Customized run option #74

EntilZha · 2014-03-17T00:18:57Z

Pull request for feature which allows customization of script run options. Encapsulates some from these issues:
#32
#6
#5

Encapsulated in this pull request is:

Additional code to create variable options for current working directory, command arguments, and script arguments. These are saved per view.
⌘-i run will use these options, which by default revert to the current behavior that ⌘-i has
⌘-shift-i runs ⌘-i, then opens the options dialog where changes can be made to the options. These options persist through runs of ⌘-i and ⌘-shift-i for a particular window.

Todo

Make the UI cleaner/more intuitive/easier. Perhaps some autosaving, collapsing the close/save into one button, ideas?
Make the options persist past the lifetime of the view via the filesystem somehow
Other stuff?

smashwilson · 2014-03-17T01:33:50Z

lib/custom-option-view.coffee

+class CustomOptionView extends View
+
+ @content: ->
+ @div class: "customOptionView", =>


Tiny style note - CSS classes are usually dash-separated.

I prefer that as well, but followed the convention of the css already there to have views camel case and everything else dash-separated.

Ah, gotcha. 😁

I think I'm the one that added the crappy camel casing the in css file. forgot what language/file i was working with 😄 feel free to update those in the css as well as yours to use the dash separation

smashwilson · 2014-03-17T01:43:31Z

Looking good 👍 Thanks for writing this, I'll definitely be taking advantage of it!

EntilZha · 2014-03-17T01:44:24Z

Thanks, I will go back and make some of those changes when I get a chance.

EntilZha · 2014-03-17T03:25:39Z

Went ahead and made those quick changes, each in their own commit.

rgbkrk · 2014-03-17T04:08:58Z

keymaps/script.cson

@@ -6,3 +6,4 @@
 'cmd-i': 'script:run'
 'ctrl-w': 'script:close-view'
 'ctrl-c': 'script:kill-process'
+ 'shift-cmd-i': 'script:options'


I think I'd prefer to call this run-options.

rgbkrk · 2014-03-17T04:23:56Z

Whoa. Just realized my view was out of sync while reviewing this. Sorry if my comments are completely off now.

rgbkrk · 2014-03-17T04:30:09Z

Thanks for reviewing @smashwilson and @EntilZha for making this new drop down.

rgbkrk · 2014-03-17T04:31:18Z

Adding image for the sake of having it in the same PR:

rgbkrk · 2014-03-17T04:36:14Z

This is going to bug me, but it's really a nitpick. In the display it reads out as "Script Arguments" though we're not limited to scripting languages (e.g. Go).

The only other thing that's missing from the dropdown (which could be added in another PR) is the command to run.

rgbkrk · 2014-03-17T04:39:02Z

lib/script-view.coffee

+ saveOptions: =>
+ @options.cmd_cwd = @customOptionView.inputCwd.val()
+ @options.cmd_args = (item for item in @customOptionView.inputCommandArgs.val().split(' ') when item != '')
+ @options.script_args = (item for item in @customOptionView.inputScriptArgs.val().split(' ') when item != '')


The only issue with splitting on space is that some args have to come in as one whole argument (the code itself being one). Then again, what else could we do? This probably is the best we're going to be able to do from a single text box.

The only other options is to populate new text boxes to each take additional args (click a +, etc.). That would probably get ugly fast. Hmm...

rgbkrk · 2014-03-17T15:53:21Z

Finally got some time to play with this. Can we change the order of operations when hitting ⌘-shift-i?

IMO, the script should only run after configuring the options so we don't give people side effects from running their script sans arguments.

On a different note, the script:toggle-options won't open unless the bottom viewer is open. I think this is an artifact of how we're instantiating.

This lack of backspace ability is so frustratingly hilarious! You have to full on exit Atom to reset the settings. Maybe in the meantime we can have a clear button that just clears all the text fields?

EntilZha · 2014-03-17T17:16:13Z

I read through the comments and like last time made the changes in more or less their own commits. I think I hit on all your feedback, but let me know if I missed something.

On the command args, I think I agree, there is not much better we can do other than assume that space division will work. I tested it with selection of text to run and it runs fine.

The backspace bug is annoying. I made a post on the atom discussion forums asking about this, hopefully get some sort of resolution there:
http://discuss.atom.io/t/input-text-element-cant-backspace/4981

Starting to look a little more polished now I think. cmd-shft-i should now pull up options whether or not script is already run and does not run the script. Everything else works as before I believe.

rgbkrk · 2014-03-17T17:36:46Z

Thanks for addressing these and refactoring this through all our reviews. Love this new view. I'll try to see who I can reach about keybindings in subviews and dig through Atom's source (grepping through /Applications/Atom.app/Contents/).

I'll also check this new version out. I'm pretty impressed already.

EntilZha · 2014-03-17T17:38:47Z

I think that the keybinding issue actually is already resolved. I missed changing the activation events keybinding when I renamed the run options. It should work without needing script already open now.

Now to get that backspace bug fixed...

rgbkrk · 2014-03-17T17:44:44Z

Sorry, I mis-spoke or wasn't clear. I meant the backspace bug when I said keybindings. The same bug is affecting copy and paste. 😭

EntilZha · 2014-03-20T02:47:02Z

Any luck on the keybinding bug? I haven't gotten a reply on the Atom forums and so far nothing from the feedback email. Is it worth making a clear button and then remove it once the bug is fixed?

rgbkrk · 2014-03-20T02:51:08Z

Yeah, I think the clear button would be fine for now. You've got something stellar here already and we'll certainly want some way to clear before releasing this.

rgbkrk · 2014-03-20T02:53:17Z

lib/custom-option-view.coffee

+ @label 'Command Arguments:'
+ @input type: 'text', class: 'editor mini editor-colors', outlet: 'inputCommandArgs'
+ @div class: 'block', =>
+ @label 'Program Arguments:'


Love this wording much better, thank you.

… a run button

… force with .hide() or .show() still. Behavior should still be the same so far

… It instead empties the panel and says its configuring options. To run the script, there is a run button which will save the options, close the option dialog, and run the program. For some reason I can't get it to appear without the script window already being open

Fixed the bug where it would not run without script already being open, which was failing to rename teh activation event from script:run-options. This now should be ready for some more review

…e script view. Currently trying to get options to pass between views

…t to persist across all views. Currently it does not, perhaps the object is copied into each view on initialization instead of referecing the same object living in script.coffee. Removed the activation events in package.json as well

… Discovered that options are not unique to a particular file, but are on an entire workspace which means they are not per file options. This needs to be addressed somehow

EntilZha · 2014-04-03T20:32:19Z

Just did the rebase, let me know if that works

rgbkrk · 2014-04-03T20:44:51Z

lib/script.coffee


 activate: (state) ->
- @scriptView = new ScriptView(state.scriptViewState)
+ @scriptView = new ScriptView(state.scriptViewState, new ScriptOptions())
+ @scriptOptionsView = new ScriptOptionsView(new ScriptOptions())


I'm a little confused here. Why don't they have the same shared ScriptOptions object?

@scriptOptions = new ScriptOptions() @scriptView = new ScriptView(state.scriptViewState, @scriptOptions) @scriptOptionsView = new ScriptOptionsView(@scriptOptions)

That is the first thing I tried. My initial thought was to create 1 object as you suggested and pass it around, with both views having access to it and essentially communicating through it.

What I found though was that the object/hash being changed in one place, did not change the other. So I concluded that it must be copying the object/hash (at the same time, the same one) for use in each view, rather than passing a reference to the same object.

To work around this, I used atom.emit and atom.on, but if this could work it would be cleaner

Alright. I must have a fundamental misunderstanding about Javascript and CoffeeScript objects then.

I'll work with this for a bit and merge it in tonight.

Sounds good. I thought that passing 1 reference around would work as well. I would be interested in seeing if perhaps I just implemented it wrong

rgbkrk · 2014-04-03T21:30:48Z

Looking good. Just tried this out, works like a champ.

Only thing missing from the options menu is a command option. We'll do that in a separate PR.

Nice work @EntilZha!

Customized run option

EntilZha mentioned this pull request Mar 17, 2014

Change Default CWD #32

Closed

smashwilson reviewed Mar 17, 2014
View reviewed changes

rgbkrk reviewed Mar 17, 2014
View reviewed changes

rgbkrk reviewed Mar 20, 2014
View reviewed changes

EntilZha added 18 commits April 3, 2014 13:18

added myself as a contributor

bad05dd

removed unnecessary return statements

5be1c21

renamed script:options to script:run-options

1e85635

Changed the text from Script Arguments to Program arguments and added…

e7e1733

… a run button

Simplified the toggle code to use jquery .toggle() and the ability to…

0bf623f

… force with .hide() or .show() still. Behavior should still be the same so far

Cleaned up more from last commit removing unnecessary default arguments

b41aab9

Added the options configuring to the script menu.

f1c5a59

Fixed the bug where it would not run without script already being open, which was failing to rename teh activation event from script:run-options. This now should be ready for some more review

Caught a bug where running wouldn't save the options first

8b320e1

Changed the naming of options to avoid a name collision

ab71393

collapsed multiple divs into one

00414e9

Refactored the options view to a better name and separated it from th…

ff2a5b9

…e script view. Currently trying to get options to pass between views

added a script options class

44c3a4f

Used atom.emit on atom.on to communicate state effectively (options).…

c9f3fd5

… Discovered that options are not unique to a particular file, but are on an entire workspace which means they are not per file options. This needs to be addressed somehow

removed unecessary action

b1a3810

added small documentation note

b05c9f5

fixes from the last rebase

c9353f4

rgbkrk reviewed Apr 3, 2014
View reviewed changes

removed uneccesary statement

8b2e31c

rgbkrk added a commit that referenced this pull request Apr 3, 2014

Merge pull request #74 from EntilZha/customized-run-option

e62d9f2

Customized run option

rgbkrk merged commit e62d9f2 into atom-community:master Apr 3, 2014

Lbatson mentioned this pull request Apr 16, 2014

Specify executable on run with cmd-shift-i #6

Closed

This was referenced Apr 18, 2014

Allow user to specify path to executable #5

Closed

Add Behat Feature support #103

Merged

rgbkrk mentioned this pull request Jul 2, 2015

Run Options should persist per project #484

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customized run option #74

Customized run option #74

EntilZha commented Mar 17, 2014

smashwilson Mar 17, 2014

EntilZha Mar 17, 2014

smashwilson Mar 17, 2014

Lbatson Mar 17, 2014

smashwilson commented Mar 17, 2014

EntilZha commented Mar 17, 2014

EntilZha commented Mar 17, 2014

rgbkrk Mar 17, 2014

rgbkrk commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

rgbkrk Mar 17, 2014

rgbkrk commented Mar 17, 2014

EntilZha commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

EntilZha commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

EntilZha commented Mar 20, 2014

rgbkrk commented Mar 20, 2014

rgbkrk Mar 20, 2014

EntilZha commented Apr 3, 2014

rgbkrk Apr 3, 2014

EntilZha Apr 3, 2014

rgbkrk Apr 3, 2014

EntilZha Apr 3, 2014

rgbkrk commented Apr 3, 2014

Customized run option #74

Customized run option #74

Conversation

EntilZha commented Mar 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smashwilson commented Mar 17, 2014

EntilZha commented Mar 17, 2014

EntilZha commented Mar 17, 2014

Choose a reason for hiding this comment

rgbkrk commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

Choose a reason for hiding this comment

rgbkrk commented Mar 17, 2014

EntilZha commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

EntilZha commented Mar 17, 2014

rgbkrk commented Mar 17, 2014

EntilZha commented Mar 20, 2014

rgbkrk commented Mar 20, 2014

Choose a reason for hiding this comment

EntilZha commented Apr 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgbkrk commented Apr 3, 2014