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

#177 Save and Load shortcut list #1276

Merged
merged 8 commits into from Nov 10, 2019

Conversation

davidlamhauge
Copy link
Contributor

@davidlamhauge davidlamhauge commented Oct 1, 2019

This PR addresses a user request from 2013.
The more you use Pencil2D, the more you know what you need from shortcuts. It is important that you can save your favourite shortcuts, so they can easily be re-instated, if they are messed up, or someone accidentally pressed 'Reset to default'.
The shortcuts are save as a text-file, and I've given them the suffix '*.p2shortcut' . It could be altered if you want to, but I prefer special and descriptive suffixes for special files.
This closes #177 .

void ShortcutsPage::saveShortcutsButtonClicked()
{
QSettings settings( PENCIL2D, PENCIL2D );
QString fDir = settings.value("LastSavePath/Animation").toString();
Copy link
Member

@MrStevns MrStevns Oct 1, 2019

Choose a reason for hiding this comment

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

As far as I can see you never set the "LastSavePath/Animation" value anywhere so the string will always be empty.

Copy link
Member

@scribblemaniac scribblemaniac Oct 1, 2019

Choose a reason for hiding this comment

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

It's set here I think:

setting.beginGroup( "LastSavePath" );
setting.setValue( toSettingKey( fileType ), savePath );

Having said that, it definitely could still be empty, which should be checked so that it doesn't try to write to the root directory.

Copy link
Member

@MrStevns MrStevns Oct 1, 2019

Choose a reason for hiding this comment

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

Hmm that looks like a different value to me, I'd expect that what you posted should equal a string "Animation" not LastSavePath/Animation

edit: or does it simply interpret that from beginGroup? anyhow I can't seem to get a value from the setting, that's why I mentioned it.

Copy link
Member

@scribblemaniac scribblemaniac Oct 1, 2019

Choose a reason for hiding this comment

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

As I understand it, if you think of the LastSavePath/Animation as a path, then beginGroup is like cd (or more accurately pushd).

Looks like it's set alright for me. if you're looking directly at the settings file, it will look like this:

[LastSavePath]
Animation=<some_path>

Take this with a grain of salt as I have not actually tested this branch yet.

Copy link
Member

@MrStevns MrStevns Oct 2, 2019

Choose a reason for hiding this comment

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

It works as expected from loadShortcutsButtonClicked but in saveShortcutsButtonClicked I never get a path. I've also debugged it a bit because the code is slightly different.

this in the load method will work:

    QSettings settings( PENCIL2D, PENCIL2D );
    QString fDir = settings.value("LastSavePath/Animation").toString();

While this in the save method will not.

settings.beginGroup( "LastSavePath" );
    QString fDir = settings.value("LastSavePath/Animation").toString();

If you however change the above to

settings.beginGroup( "LastSavePath" );
QString fDir = settings.value("Animation").toString();

Then I get the expected behaviour.

Copy link
Contributor Author

@davidlamhauge davidlamhauge Oct 2, 2019

Choose a reason for hiding this comment

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

Maybe that's what is not working in Ubuntu, but is working on Win and iOS. As explained in another comment, I have som odd behavior on my Ubuntu. I'll test it when I get home.

Copy link
Member

@MrStevns MrStevns left a comment

This looks good, I've only made one observation that seems strange to me.

I have a comment on the extension name though, to try to stay a little more consistent with our previous extension names, I think it would be better to call it *.pclshortcuts but it's a minor thing.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Oct 2, 2019

Great work. I understand the need for descriptive names, but If possible let's try not to exceed the 3~6 character range for filename extensions. I know it's not 1980 but it's better to make it short for mnemonics and simplicity. We already have *.vec, *.pcl, *.xml, etc so hopefully we can continue being coherent. Nevertheless here are some varied proposals:

  • *.key
  • *.ksc (keyboard shortcut)
  • *.pclkey
  • *.kbdmap
  • *.keymap

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Oct 2, 2019

I have tested it thoroughly, and it finds the last animation path as expected.
I can see that I forgot these lines
if (fDir.isEmpty()) fDir = QDir::homePath();
in the loadShortcutsButtonClicked, so the path can be empty if nothing has been saved before. I'll correct that later today.
About the name, the shortest I can think of is *.pclscut, but for the sake of the user I still would prefer *.pclshortcut. I have no issues with long suffixes. Especially not when they are helpful.
I just remebered one odd thing: I've tested it on MacBook, win10 and Ubuntu18.04, and it works as expected, except when I want to load a shortcut file in Ubuntu. Here it finds the last saved animation path, but the filter is not working. It shows all files, and the button that should show the filter is empty. When I doubleclick this button, the filter starts working, the text on the button appears, and everything is ok again. Can any of you tell me why it's like that? It's work fine in iOS and Win.

@chchwy
Copy link
Member

chchwy commented Oct 2, 2019

The file is actually in INI format, so call it *.ini is legit, too.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Oct 2, 2019

Yes, it is the INI format, but it's content is not a typical ini-file, so I would not support that. If the others support it, iI will of course follow the majority.
This reminds me of the palette files that have the *.xml suffix. Even if the content is xml, I would never have chosen that name. I would go with *.pclpalet or something. When I export a palette file, I start the filename with 'PAL', as in PALgeorge.xml, so I can see that it is a palette file. There are so many xml-files around that I have to distinguish it somehow.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Oct 2, 2019

With some help I go the path problem solved. Everything works as expected in my tests now.
Since I was making a commit anyway, I changed the suffix to *.pclshortcut. I still think a suffix to such a file should be descriptive, but I'll change it again if necessary, to whatever we can agree upon.

@scribblemaniac scribblemaniac added the 🔹 Minor PR (only one reviewer required) label Oct 3, 2019
Copy link
Member

@MrStevns MrStevns left a comment

LGTM 👍

Do we need further discussion on the extension name? I have no requirements aside from making it obvious that it's a pencil format.. eg. pcl should be included. I would be okay with pclkey if @Jose-Moreno wants to keep it short.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Oct 4, 2019

I think the same as @candyface that it should be related to the software using pre-existing format keywords . Although I would certainly like it to be shorter, if it's not possible or usable then just go with *.pclkey[s] (plural might work better) or for more descriptive power try with *.pclkeylist or *.pclkeymap Whatever works is fine with me. Good work and thank you for doing this @davidlamhauge ! 😊

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Oct 4, 2019

I can't understand why *.pclkeymap, *.pclkeys or *.pclkeylist is better than *.pclshortcut, when it comes to a file that contains nothing but information on shortcuts.
Of the three suggestions, I prefer *.pclkeys, but I'm glad that I know it's a shortcut file, because I would never have guessed it by the name.
Can we agree on *.pclkeys ?

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Oct 4, 2019

Sorry, I just wanted to understand and suggest an appropriate name, so let me analyze this last piece here then.

Is shortcut more universal than keymap? is it more common? We do use "keyboard shortcuts", keymap is perhaps more technical jargon. Key can be confused with keyframe, so there's that.

In that sense I honestly don't really mind if it's renamed as *.pclshortcuts , but I must also apologize for the misunderstanding, I was working under the assumption that it was still proposed to be p2shortcut instead 🤦‍♂️ 🙇‍♂️

Either way I see there is no possibility we can have both a descriptive name and short name for the extension, so, let's go with the shortcuts one and let this PR be finalized under the assumption that:

a. This is a file type that will be directly manipulated by the end user and read by Pencil2D
b. The extension is honestly only for cosmetic purposes
c. We are assuming the user has filename visibility enabled in their operating system (which most people don't)
d. In a common example, I assume everyone will rename the shortcut list to something along the lines of "Pencil2DShortcutList_Animation.pclshortcuts" but most people won't see the extension. and those who do will adjust the name accordingly to avoid longer names. As I can see filenames, I'd rename them like "Drawing.pclshortcuts" or "Coloring.pclshortcuts" if that were the case

More error checking and reporting (debug only) for shortcut loading.
Also used beginGroup to prevent the possible manipulation of other
keys.
Copy link
Member

@scribblemaniac scribblemaniac left a comment

I thought that it would be more appropriate to use the built-in QSettings to write the ini file in addition to reading it. I also thought just by reading the code that any setting could be replaced by including in the shortcut file which would be bad, however in testing I found that this doesn't actually happen unless the program is killed or crashes because the preference manager overwrites any other settings, still I think we should prevent that anyway.

I fixed these things and also added also altered the loading behavior slightly: it will check for formatting errors, and it will not delete all shortcuts, only overwrite them so that any shortcut keys that are not in the file you are loading are not deleted from the settings. Let me know what you think of that in particular. Setting a key to the default value if not found would also be a reasonable behavior, but I like this more because it theoretically allows you to only alter some settings.

I meant to push these changes to my fork, but accidentally pushed them to yours @davidlamhauge 😱. Please review it and let me know if it's good. If not it can always be removed or changed. Sorry about that!

@scribblemaniac
Copy link
Member

scribblemaniac commented Oct 5, 2019

The code looks good aside from what I have already noted/fixed. I don't like how long .pclshortcuts is but I like shortcuts over keymap. I personally would have gone with .pcls for shortcuts and .pclp for palettes, but I don't care too much. Whatever we settle on, it would be a good idea to add it into to the mime type lists (ex. info.plist, desktop file), but only after we support opening these files from the program arguments.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Oct 5, 2019

I haven't tested it @scribblemaniac , but it looks good. What now? I can't see that I can merge it, since it's not a PR.
About the suffix, I think *.pcls is a good alternative to the *.pclkeyxx combinations, which I honestly don't like. What do you say @Jose-Moreno ?
Next thing is to alter palette suffixes from *.xml to *.pclp, but I guess that's too late...

@scribblemaniac
Copy link
Member

scribblemaniac commented Oct 5, 2019

I haven't tested it @scribblemaniac , but it looks good. What now? I can't see that I can merge it, since it's not a PR.

You don't have to do anything if you're good with it since it was already push to your repository. You'll want to run git pull on your local repository to get the changes if you haven't already of course. Please do test it when you get a chance, I'm only able to test it on linux right now, and as you know things don't always work the same on other systems.

Next thing is to alter palette suffixes from *.xml to *.pclp, but I guess that's too late...

We could still change it. We would still have to allow opening .xml files but we could write all new files with a different extension at least.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Oct 5, 2019

I don't mind *.pcls +1 👍

Regarding *.xml that is a bit more trickier for me, I feel this format, and others similar to it, should be used as a data exchange format.

I've always entertained the idea of allowing software like Harmony, Synfig, GIMP, Krita, etc to import the Pencil2D palette back and forth. Changing this file format name to something specific might confuse other people trying to get the information out of pencil2d.

That said, if we leave the capability of having both, the *.pclp for internal purposes and the *.xml for exchange purposes, with a more universal node structure, I'm fine with that too.

Don't know what the others think about the latter idea, but if you guys want to change the palette format either way you have my blessing 😋

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Oct 5, 2019

I've tested it @scribblemaniac , and it works fine, except line 152 and 154. With these lines in the code, the odd behaviour explained 4 days ago, and solved by @candyface , comes back, so I've deleted these lines. It's tested on Ubuntu, so I wonder why you didn't have the same odd behaviour?
About the *.xml for palettes, I think we should discuss it elsewhere, but I think it should be discussed.

@scribblemaniac
Copy link
Member

scribblemaniac commented Oct 5, 2019

Regarding *.xml that is a bit more trickier for me, I feel this format, and others similar to it, should be used as a data exchange format.

I think basically the opposite, that it should primarily be a internal format and a more widely recognized format like the GIMP palette format should be used for interchange. Anyway, as @davidlamhauge said, this can be discussed elsewhere.

I've tested it @scribblemaniac , and it works fine, except line 152 and 154. With these lines in the code, the odd behaviour explained 4 days ago, and solved by @candyface , comes back, so I've deleted these lines.

Huh? Line 152 was added by @candyface to fix the issue in the first place, and 154 is after the setting is retrieved so it should not affect that at all.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Oct 6, 2019

@scribblemaniac It still yielded some strange behaviour on my Ubuntu, so now I've done what I probably should have done from the beginning. I've added Shortcuts to LastSavePath in Preferences.

@MrStevns
Copy link
Member

MrStevns commented Oct 6, 2019

I think that *.pcls is too close to the project file format, even if it's for internal use only I still think the extension should be somewhat descriptive and separated from other potential name clashes. I'd rather have *.pclkmap, *.pclkeymap or *.pclkeys

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Oct 6, 2019

My favourite office suite, LibreOffice, uses the odf format. Writer documents are called *.odt, Spreadsheets are called *.ods and Presentations are called *.odp. I have never heard anyone complain about mixing them up.
Word has used *.doc for many years. Now they use *.docx, and *.dotx if it's a template, without problems afaik.
As mentioned before, I like the idea of having longer and more descriptive extensions, but *.pclkmap, *.pclkeymap and *.pclkeys confuse me more than the enligth me.
*.pclshortcut is my preferred, if we can't agree on *.pcls.

@MrStevns MrStevns added this to the 0.6.5 milestone Nov 10, 2019
@MrStevns MrStevns merged commit 32fff24 into pencil2d:master Nov 10, 2019
@Jose-Moreno Jose-Moreno mentioned this pull request May 2, 2020
56 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request 🔹 Minor PR (only one reviewer required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A way to save the Custom shortcuts list for future needs.
5 participants