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

Bring me updates #614

Merged
merged 19 commits into from
Jul 2, 2019
Merged

Bring me updates #614

merged 19 commits into from
Jul 2, 2019

Conversation

mrob95
Copy link
Member

@mrob95 mrob95 commented Jun 27, 2019

I've tidied up this code following the basic pattern of #577 , using a single object to keep track of config rather than calling a bunch of separate functions.

I've also implemented context dependent folder actions for terminal and Windows Explorer as mentioned in #476, so "bring me my documents" will "cd" if a terminal window is active, and will use a focused file explorer window instead of creating a new one.

Happy to get feedback on whether or not people think this is the right approach.

@mrob95 mrob95 self-assigned this Jun 27, 2019
@mrob95 mrob95 added Enhancement Enhancement of an existing feature Testing Required Requesting testers before merge labels Jun 27, 2019
@LexiconCode
Copy link
Member

I think it's a very reasonable way to go as it reduces duplicate commands and provides a standard workflow. It would be interesting to see what other areas we can brainstorm to expand this functionality further for the roadmap.

Did you run into this issue while working on the Bring me updates?
#575

@LexiconCode
Copy link
Member

LexiconCode commented Jun 27, 2019

This is a good example of a SelfModifyingRule as it is very easy to read. I appreciate that as many of Caster existing SelfModifyingRule examples are convoluted.

@LexiconCode
Copy link
Member

LexiconCode commented Jun 29, 2019

A significant improvement and definitely much easier to read code.

Everything works the expected except for "restore bring me defaults" producing no error message or restoring the default bringme keys. A not as clean workaround is to overwrite the bringme.toml with the default instead of trying to load the defaults from the source location. Overall I prefer the second method.

Edit: "restore bring me defaults" does load the defaults. bringme keys that are default function as expected however they are not saved back into bringme.toml and if bringme.toml is deleted the files not restored.

One suggestion is to include the context for windows powershell as well.

@mrob95
Copy link
Member Author

mrob95 commented Jun 29, 2019

Regarding defaults, I think it makes sense to define them in a dictionary in the same file rather than a separate toml file. Mainly because it keeps everything in one place and easily readable.

I've also modified the functionality so that restoring defaults also overwrites the config file. Thoughts?

@LexiconCode
Copy link
Member

LexiconCode commented Jun 29, 2019

Regarding defaults, I think it makes sense to define them in a dictionary in the same file rather than a separate toml file. Mainly because it keeps everything in one place and easily readable.

I've also modified the functionality so that restoring defaults also overwrites the config file. Thoughts?

Perfect a bit of clean up to remove the bringme default path/bringme.toml.defaults/ the function that restores bringme.toml.defaults to the data directory.

@LexiconCode
Copy link
Member

LexiconCode commented Jun 29, 2019

After further testing #575 is an issue however I'm not sure if that's what you want in the scope of this pull request.

Second thoughts on a spec change "<launch> to bring me as <key>": is difficult for me to say. "bring me <launch> as <key>" or even "<launch> bring me as <key>" seems much more natural. Thoughts?

It's not an issue for me personally as I can modify it through filter rules.

@LexiconCode
Copy link
Member

LexiconCode commented Jun 29, 2019

Another concept that I thought interesting. alias <launch> as <key> to save alias which is essentially what "<launch> to bring me as <key>" is performing just within a different context.

The command bring me would be used across caster to retrieve x . alias would be used to save X as Y across caster.

@mrob95
Copy link
Member Author

mrob95 commented Jun 29, 2019

I'll think about it, can probably use a context action for this as well

@LexiconCode
Copy link
Member

LexiconCode commented Jun 29, 2019

This is debatable but possibly functional for those that manually edit the bringme.toml by preference or testing purposes.

            "refresh bring me":
                R(Function(self.refresh)),

Your new additions test out and function as expected.

@kendonB
Copy link
Collaborator

kendonB commented Jun 29, 2019

All of these additions seem to work well. What do you think about adding the following:

"bring me <desired_item> new"

This one would always open a new window to open the desired item. If the current context is a terminal, folders would open in a new terminal window.

"bring me <folder_item> terminal"
"bring me <folder_item> [windows] explorer"

These would operate by 1) looking for an open instance of one of these types of windows, 2) navigating to that folder in an open instance if one exists; if one does not exist, opening a new window at that folder.

"bring me <folder_item> new terminal"
"bring me <folder_item> new [windows] explorer"

Like the above but it always opens a new window.

@kendonB
Copy link
Collaborator

kendonB commented Jun 29, 2019

Would also be good to incorporate all of this into the file_dialogue context. This would incorporate ideas from #459

@LexiconCode LexiconCode reopened this Jun 30, 2019
@mrob95
Copy link
Member Author

mrob95 commented Jun 30, 2019

Okay I've done another rearrange and implemented some of the above. Websites and programs/files work the same but for folders:

  • bring me <folder> is as before, context-dependent. If you are in an Explorer or terminal window it will use that, otherwise it will open a new Explorer.
  • bring me <folder> in explorer opens in a new Explorer window regardless
  • ditto for bring me <folder> in terminal

I'm not a powershell user so not sure how well this will work for that.

I think having a separate method for each item type is neater and will make for easier scaling if we need to add more categories.

@LexiconCode LexiconCode changed the base branch from develop to master June 30, 2019 20:43
@mrob95 mrob95 added the Documentation Needed Issue needs grammars or function documentation label Jun 30, 2019
@LexiconCode LexiconCode removed the Documentation Needed Issue needs grammars or function documentation label Jul 1, 2019
Updated hyperlink for caster and dragonfly. 
Tweaked order of keys
For Caster and dragonfly add "documentation <project>" per project.
@LexiconCode
Copy link
Member

LexiconCode commented Jul 1, 2019

@mrob95 I made a few tweaks and if you don't feel of the relevant they can be removed.

  • Updated hyperlink for caster and dragonfly.
  • Tweaked order of keys
  • For Caster and dragonfly modified "<project> documentation" to call documentation per project.

@LexiconCode
Copy link
Member

LexiconCode commented Jul 1, 2019

A bug for those that don't use the default location of my documents or any other configurable library.
My Documents on my system is pointed to an alternate location.
image

Therefore with command prompt/powershell context with bring me my documents produces unexpected file path when it should be D:\Backup\Library\Documents

C:\Users\Main>cd "C:\Users\Main\Documents"
The system cannot find the path specified.

When dictating bring me my documents out of the above context the command opens up my documents folder as expected. However Bring: bring me <folder> [in <app>], C:\Users\Main\Documents, None as the path is not correct. I would not expect this to function correctly if the path was not correct.

There is not an easy solution due to there is no environmental variable for my documents. I've experienced some goofy behavior with environmental variables with Python.(I checked and this is not the problem in this case).

Another alternative with power shell and you could grab [environment]::getfolderpath("mydocuments") to get the correct path.

Edited @mrob95: demo code

def check_output():
    try:
       output = subprocess.check_output(
                  ["powershell.exe", "[environment]::getfolderpath('mydocuments')"], 
                  shell=True)
    except subprocess.CalledProcessError, e:
        print "subproces CalledProcessError.output = " + e.output
    print output
check_output()

Prints D:\Backup\Library\Documents

@LexiconCode
Copy link
Member

You may not be aware but you could leverage these as well.

Dragonflies runcommand-action
and
Caster's TerminalCommand

@mrob95
Copy link
Member Author

mrob95 commented Jul 1, 2019

Hmm, I'm surprised that it works in one context but not in another. Maybe try adding the following to _rebuild_extras to see what is going on:

print({key: os.path.expandvars(value)
            for key, value in section.iteritems()}
            for header, section in self.config.iteritems())

I'm aware of RunCommand but Popen() is actually cleaner than RunCommand().execute(). Happy to change it though if we want to get rid of Popen across the code base.

@LexiconCode
Copy link
Member

LexiconCode commented Jul 1, 2019

_rebuild_extras

I assume you mean _rebuild_items

    def _rebuild_items(self):
        # E.g. [Choice("folder", {"my pictures": ...}), ...]
        print[(header, {key: os.path.expandvars(value)
            for key, value in section.iteritems()})
            for header, section in self.config.iteritems()]
        return [
            Choice(header, {key: os.path.expandvars(value)
            for key, value in section.iteritems()})
            for header, section in self.config.iteritems()
        ]
(u'folder', {
u'my documents': u'C:\\Users\\Main\\Documents', 

Edit @mrob95 if I type C:\Users\Main\Documents into explorer.exe address it goes to the my documents. I think neither context is getting the correct path however the Explorer.exe context is a smarter way of handling it versus cmd.

I'm aware of RunCommand but Popen() is actually cleaner than RunCommand().execute(). Happy to change it though if we want to get rid of Popen across the code base.

I would say it's not as clean, However think there's also utility and advantage to leveraging RunCommand Therefore if your happy to change Popen across the code base go for it.

kendonB added a commit to kendonB/Caster that referenced this pull request Jul 1, 2019
@mrob95 mrob95 merged commit 7994cda into dictation-toolbox:master Jul 2, 2019
@LexiconCode LexiconCode removed the Testing Required Requesting testers before merge label Jul 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Enhancement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants