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

Desktop Mode Save Action Behaviour #257

Closed
leegrey opened this issue Mar 12, 2015 · 11 comments
Closed

Desktop Mode Save Action Behaviour #257

leegrey opened this issue Mar 12, 2015 · 11 comments
Milestone

Comments

@leegrey
Copy link
Contributor

leegrey commented Mar 12, 2015

Hi. I am really enjoying using Piskel, and am using the Desktop / Node-Webkit version.

It seems that you are detecting when you are running in Node-Webkit and have a different save behaviour (select file path and name), which is great. I was thinking that it would be nice to extend this to behave more like a full native app when running on the Desktop.

I was thinking that what is needed is a "Save As" action, that would remember the chosen filename, and would allow a faster re-save to that same file. I see that it is in your roadmap, but I'd really like to see a ctrl-s save keypress. That could save either into the browser in the web version, or in Desktop mode, into the target file path.

I'm always compulsively hitting ctrl-s in every app I use, and I miss it in piskel :)

I'm quite familiar with node.js and Node-Webkit, and if I can find the time I'd like to contribute in this area if I can. Have you had any thoughts on similar lines? It's not the most glamorous feature, but it would make the offline workflow smoother.

@juliandescottes
Copy link
Collaborator

Hi !

I would love to improve the 'save' flow, so totally in favor of working on this (I also miss the ctrl+S !)

The 'Save as File' feature is relying on a link with a download attribute. In node-webkit, this triggers the 'Save As' popup automatically. There is no specific implementation :) I assume it's the default behavior of node-webkit for download links.

Looking at the documentation, node-webkit can use all the Node.js APIs, including the filesystem API (https://nodejs.org/api/fs.html). So technically, this looks feasible.

If we can do a good file-based save system, we can simplify the UI a lot for the node webkit version (remove the name/description form, remove the save offline section ...).

@leegrey
Copy link
Contributor Author

leegrey commented Mar 13, 2015

If we can hold onto the full save file name and path, then we can easily bind ctrl-s to a re-save action. (Rather than opening the dialog again and timestamping the file name, as it does for the web version.)

I should have time this weekend to try it, so I'll let you know how it goes. Thanks. :)

@leegrey
Copy link
Contributor Author

leegrey commented Mar 14, 2015

Hi. I've made some progress. I've added a util for detecting when running in Node-Webkit. When Node-Webkit is detected, the save action opens a dialog using a different system that is able to acquire the full path to the file. The file is then saved using the node file system api. (By the way, the dialog can be opened in the browser too, and I think it could be used to select the save destination, if you wanted that feature in the browser version.)

The next step is to hold on to the file path between saves, and skip the save dialog next time the save action is triggered. I realise that this is potentially dangerous, so I need to be able to check that we are definitely saving the same document and not a new one. Are their any events, flags, or document ids I could use to check for that?

I started adding ctrl-s functionality into SaveController, but I notice that it is only initialised when you expand the save panel. It works after that though. I'm still working out the structure of the project, so it's just sitting in SaveController for now.

If you'd like to take a look at the work-in-progress, you can check out the "desktop-save-action" branch in my fork:

https://github.com/leegrey/piskel/commits/desktop-save-action

@juliandescottes
Copy link
Collaborator

(almost no computer time these days, don't worry if I don't respond quickly :) )

Are their any events, flags, or document ids I could use to check for that?

I don't think we can reuse anything here. I suggest we store the save-path on the Piskel instance stored in pskl.app.piskelController. Then we need to review where we create a new Piskel instance and if this save-path should be propagated to the new instance. For example :

  • in History Service (undo/redo), when we recreate a Piskel instance, the save-path should be kept
  • in Import Controller when we import a piskel from a file or an image, save-path should be reset

What do you think ?

SaveController only initialized when opening settings panel

For now you can leave the code here. We should split this file into a Controller dedicated to handle events coming from the Save settings panel, and a Service instantiated at application startup, with the save logic + keyboard listeners.

I'll have a look at your fork during the week !

@leegrey
Copy link
Contributor Author

leegrey commented Mar 15, 2015

Thanks for the feedback. I've continued to read through the code and am starting to have a clearer understanding of how things work. I was thinking the same thing about the SaveController, since ctrl-s etc need to work before the save panel is opened.

I'll try out your suggestion about storing the save path this week.

@leegrey
Copy link
Contributor Author

leegrey commented Mar 17, 2015

I've implemented the storing and propagation of the savePath as you suggested. I've also added a Node-Webkit based file load function so we can hold onto the full savePath when loading a piskel file. Everything seems to be working nicely. The main issue now is that the keyboard shortcut listener for ctrl-s is not initialised until the first time you open the save panel. I'll create a service for this as you suggested, and move some of the save logic out of SaveController.

@juliandescottes
Copy link
Collaborator

Looks great !

May I ask about your development environment for node-webkit ? Do you build the app every time you need to test a change or is there a better way ?

@leegrey
Copy link
Contributor Author

leegrey commented Mar 17, 2015

I'm just using a standalone version of Node-Webkit, pointing at the /src folder. It uses the "?debug" query string to use the non-packaged source, just like in the "grunt server:debug" configuration. Node-Webkit has the chrome developer tools turned on by default, so development is basically the same as in the browser, no build required. Just hit refresh to see changes.

@juliandescottes
Copy link
Collaborator

I just had a look at your fork and did a quick compare with the current master.
Everything looks fine and inline with the project => feel free to submit a PR whenever you're ready :)

Thanks !

@leegrey
Copy link
Contributor Author

leegrey commented Mar 23, 2015

Will do! I just wanted to give it a once over before making a Pull Request. (I've been too distracted making pixel art!) I'll do that tonight. Thanks :)

@juliandescottes juliandescottes added this to the v0.5 milestone Mar 25, 2015
@juliandescottes
Copy link
Collaborator

Released in v0.5.0. Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants