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

Reintroduce toolbars #300

Merged
merged 21 commits into from
Aug 2, 2020
Merged

Conversation

nbrunett
Copy link
Contributor

Here's a start to adding toolbars back, like I briefly described in #188. I mainly just went through the Notes and Tab Symbols menus. Shout if there is something missing or something that should be removed.

They use the Commands already in the menus so it was just a matter of adding the toolbars. Specific widgets could be built for greater control of the appearance with the Commands just plugged into the buttons they contain. All of these buttons (except the dynamics, see below) work at this point.

It should be possible to have toolbars side-by-side when they are placed on the sides of the main window. I can do it manually on my Mac, but the ToolBarBreaks do not do this automatically for me when the application starts.

I expect there is a better place to set the Command icons than what is here now. Recommendations would be appreciated, or I'll dig around a bit to see if I can figure this out next.

I added dynamic buttons that don't do anything yet. I was working on adding a PowerTabEditor::updateDynamic method that these would connect to, but I ran out of patience for now with some constructor compile errors. I'll poke at this a little more and then add what I have so far as a call for help.

@cameronwhite
Copy link
Member

Thanks for submitting this!

If you need any help with the updateDynamic errors, let me know!

I think I'd probably set the icons at the same time that the Command is constructed, either with an optional argument in the constructor or just calling setIcon() immediately afterwards. Setting iconVisibleInMenu to false for all Commands might be a good idea since some of the icons look pretty squished in the menu bar.
The toolbar icons look a bit on the large side (at least on my Mac) so I'd maybe try seeing if a smaller size looks good!

I do like how with the toolbar approach you can move or hide any of the sub toolbars pretty easily! I'd say the downside I've seen is that if you arrange multiple toolbars beside each other vertically (because you ran out of vertical space), the layout looks odd versus e.g. Guitar Pro's toolbar where related items are near each other in both the vertical and horizontal directions

@nbrunett
Copy link
Contributor Author

Icon file paths are an optional parameter to the Command constructor now.

I have also set the max. size of icons in the toolbars.

Up to 3877e34 compiles for me, but the commit after that does not. That is where I try to add the dynamics updating stuff. It gives the errors:

../source/app/powertabeditor.cpp:1378:33: error: no matching constructor for initialization of 'EditDynamic'
        myUndoManager->push(new EditDynamic(location, volume, dynamic),
                                ^           ~~~~~~~~~~~~~~~~~~~~~~~~~
../source/app/../actions/editdynamic.h:28:5: note: candidate constructor not viable: no known conversion from 'const Dynamic *' to 'const Dynamic' for 3rd argument; dereference the argument with *
    EditDynamic(const ScoreLocation &location,
    ^
../source/app/../actions/editdynamic.h:25:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 3 were provided
class EditDynamic : public QUndoCommand
      ^
../source/app/powertabeditor.cpp:1387:37: error: no matching constructor for initialization of 'AddDynamic'
            myUndoManager->push(new AddDynamic(location, dynamic),
                                    ^          ~~~~~~~~~~~~~~~~~
../source/app/../actions/adddynamic.h:28:5: note: candidate constructor not viable: no known conversion from 'const Dynamic *' to 'const Dynamic' for 2nd argument; dereference the argument with *
    AddDynamic(const ScoreLocation &location, const Dynamic &dynamic);
    ^
../source/app/../actions/adddynamic.h:25:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
class AddDynamic : public QUndoCommand
      ^
2 errors generated.

I'm a bit rusty with C so it's probably something silly, but I thought I followed your example from things like adddynamic and editnoteduration pretty well. Any suggestions on what to change or what to try?

Thanks.

Copy link
Member

@cameronwhite cameronwhite left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I've left a couple comments for how I think the compilation errors can be fixed.

source/app/powertabeditor.cpp Outdated Show resolved Hide resolved
source/app/powertabeditor.cpp Outdated Show resolved Hide resolved
source/app/powertabeditor.cpp Outdated Show resolved Hide resolved
source/app/powertabeditor.cpp Outdated Show resolved Hide resolved
@nbrunett
Copy link
Contributor Author

nbrunett commented Jul 7, 2020

I think the comments so far have been addressed, except for the issue of moving toolbars around potentially ruining some of the button grouping. Would you like me to start looking into making widgets that emulate the GP layout instead of using the default toolbars now?

Copy link
Member

@cameronwhite cameronwhite left a comment

Choose a reason for hiding this comment

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

Thanks, these changes look good! Just a couple minor notes in the comments.

I think it would be good to start looking at a custom layout since the toolbars seem too limited in that area. Both GP and also MuseScore would be some good inspiration!

source/app/powertabeditor.cpp Outdated Show resolved Hide resolved
source/app/powertabeditor.cpp Outdated Show resolved Hide resolved
@nbrunett
Copy link
Contributor Author

It's not pretty or sophisticated, but the buttons are now all in a dock and they work as far as I can tell. The toolbar has been removed too. I think my main questions now are:

  1. Do you want to use an actual toolbox, so there are collapsible pages? I personally don't like this because I think it makes for more clicking to find the button you want, but that's just me.
  2. What's the process for getting new icons for the buttons that currently don't have ones?

@cameronwhite
Copy link
Member

Great! I think it's looking good as-is - adding extra nesting doesn't seem necessary since there aren't enough buttons to run out of room (at least on my screen)

I think the current icons were just pngs generated from the music notation font or some simple drawings in an image editor. I can take a look at putting some together!

@cameronwhite
Copy link
Member

Here are a few new icons: new_icons.zip. Let me know if they are usable, and then I can work on the remaining ones :)

@nbrunett
Copy link
Contributor Author

Those look good. Both PNG and SVG work.

@cameronwhite
Copy link
Member

Great! I think I've got all the remaining ones in this version:
new_icons.zip

@cameronwhite
Copy link
Member

Do you have any remaining todos now that the icons are in, or is it ready to merge?

@nbrunett
Copy link
Contributor Author

Oh sorry, I'm a bit on the fence about calling it done now or trying to find and add a few buttons to fill out the rest of the rows that aren't 8 across.
Screen Shot 2020-07-23 at 7 16 12 PM

I should probably fill out the rest of the grid so what gets merged looks decent, as long as you're willing to supply some more icons. Sorry about not figuring it out ahead so you could just do it all in one batch. I can get you the list of remaining buttons for icons next.

@cameronwhite
Copy link
Member

No problem! Let me know when you need any icons

@nbrunett
Copy link
Contributor Author

How about icons for these?

  • Key signature
  • Time signature
  • Barline (for "Insert Standard Barline")
  • Repeat (for "Repeat Ending")
  • Tempo
  • Multi-bar rest
  • Left-hand fingering
  • Metronome
  • Text
  • Musical direction (coda symbol?)
  • Rewind

@cameronwhite
Copy link
Member

Sure, I'll look at those later this weekend!

@cameronwhite
Copy link
Member

Here are some more icons!
new_icons_2.zip

I'm not sure about the rewind and metronome options being in the toolbox though, since they're already in the main playbar widget

@nbrunett
Copy link
Contributor Author

nbrunett commented Aug 1, 2020

Well I was going to call this done with the last two commits, but trying to resolve the conflicts with master has apparently broken things. I will do some investigating to see if I can sort this out.

@nbrunett
Copy link
Contributor Author

nbrunett commented Aug 2, 2020

I think I've done all I can for the build checks. The windows failure has something to do with installing dependencies. I'm ready to merge when you are.

@cameronwhite
Copy link
Member

Thanks! I'll merge this in shortly.
The windows failure is from actions/runner-images#1332 which is currently affecting the main branch too

@cameronwhite cameronwhite merged commit ddde336 into powertab:master Aug 2, 2020
@cameronwhite cameronwhite mentioned this pull request Aug 2, 2020
@cameronwhite
Copy link
Member

Thanks for the contribution!

@nbrunett nbrunett deleted the reintroduce-toolbars branch August 15, 2020 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants