MuseScore icon indicating copy to clipboard operation
MuseScore copied to clipboard

fix #13286 - ensure no empty error message shown when adding text and nothing selected

Open wizofaus opened this issue 1 year ago • 19 comments

Resolves: (direct link to the issue)

(short description of the changes and the motivation to make the changes)

  • [x] I signed CLA
  • [x] I made sure the code in the PR follows the coding rules
  • [x] I made sure the code compiles on my machine
  • [x] I made sure there are no unnecessary changes in the code
  • [x] I made sure the title of the PR reflects the core meaning of the issue you are solving
  • [x] I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • [x] I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • [ ] I created the test (mtest, vtest, script test) to verify the changes I made

wizofaus avatar Sep 11 '22 04:09 wizofaus

@wizofaus Your issue is a duplicate of #13248, and this PR is a duplicate of #13280 😅

cbjeukendrup avatar Sep 11 '22 11:09 cbjeukendrup

I actually did think so. But his solution was different from mine.

HemantAntony avatar Sep 11 '22 12:09 HemantAntony

I did search but couldn't find anything. Hemant's solution will show the same "select a note or rest" message in every case you add any type of text element and nothing is selected, which wasn't an assumption I thought it was safe to make.

wizofaus avatar Sep 11 '22 19:09 wizofaus

@HemantAntony / @wizofaus - are you in agreement about which of the issues we should choose to test?

Tantacrul avatar Sep 12 '22 21:09 Tantacrul

Actually I'm not convinced that either solution covers all cases properly - really that function to determine if you can add a text element based on the current selection should never return just "false", or you'll get the same empty message. I'd prefer to change this one to ensure that's never possible.

wizofaus avatar Sep 12 '22 22:09 wizofaus

In particular it's still showing an empty message if you use the "add text frame" command and no horizontal or vertical frame is selected. Normally that's not possible because the command only appears in the right-click context menu for h/v frames, but you can define a shortcut for it and activate it that way too, which can give rise to the empty dialog message.

I also fixed a crash that occasionally happened when that context menu was displayed.

wizofaus avatar Sep 13 '22 01:09 wizofaus

@bkunda - when you have time, can you test this and let me know if you're happy that it solves your 'don't show an empty error message' issue?

Tantacrul avatar Sep 13 '22 08:09 Tantacrul

@bkunda - when you have time, can you test this and let me know if you're happy that it solves your 'don't show an empty error message' issue?

Yes this fix does the job 👍🏻

I'd really prefer it if the text formatting could be corrected for both this and also the dialog with the "figured bass" alternate messaging (if this is possible):

Current Screenshot 2022-09-19 at 12 50 11 pm

Preferred Screenshot 2022-09-19 at 12 51 20 pm

But I seem to recall an earlier discussion where this was deemed not possible, in which case this PR would have to be accepted as-is.

bkunda avatar Sep 19 '22 10:09 bkunda

In particular it's still showing an empty message if you use the "add text frame" command and no horizontal or vertical frame is selected.

I did notice that no messaging dialog is triggered for frames. The corresponding dialog in MS3 bore this message:

Screenshot 2022-09-19 at 12 58 10 pm

For now I'd be happy if this messaging could be copied-over to the new dialog style.

bkunda avatar Sep 19 '22 11:09 bkunda

But "bar" is wrong, there's no requirement a measure be selected before adding a frame. Not sure what you mean about "note or figured bass", are you supposed to be able to add figured bass when an existing figured bass direction is selected?

Edit: oh ok, that IS the current message (which seems a bit odd to me, but anyway), it was the formatting you were commenting on. BTW why does this dialog even have "don't show again"? That just leaves you with a situation that you choose a command...and nothing happens, which is about the worst UX you can give surely? Though at least it then becomes consistent with a number of other commands (add interval etc.) that do nothing if the wrong thing is selected.

wizofaus avatar Sep 19 '22 11:09 wizofaus

The instruction to "please select a bar" is not incorrect. If the user follows this instruction, they will be able to achieve their goal of inserting a frame. Certainly, it isn't the only element they could select to insert a frame, but listing out every element they could potentially select would be cumbersome and unhelpful, and trying to generalise the message could potentially lead to unintended results (E.g. "please select something...", which could see the user selecting an element that doesn't permit a frame).

It's not brilliant messaging, but it's a reasonable solution and I don't think there's any great danger in keeping it for now.

As for the figured bass, I've had another look at how this works and another solution here – perhaps a simpler one – would be just to have the one dialog for all these text elements (I.e. "please select a note or rest..."). You can select a figured bass number and then Add > Figured bass, but this doesn't add a figured bass so much as enter you into edit mode for the selected figured bass. So having one consistent dialog for all text elements here (Or rather, not having an exceptional dialog for figured bass) would actually make more sense.

And yes, my comments above concerned the UI. Incidentally, the "don't show again" was considered a courtesy for users who found this kind of prompt frustrating. And we weren't too worried about the UX here because the user would be aware that they've elected to not show this type of message. However if this generates a lot of confusion then of course we are always open to tweaking the UX once we get more feedback from users.

bkunda avatar Sep 19 '22 13:09 bkunda

I would also mention, in the past it always was necessary to select a full bar when inserting a frame. Which makes some logical sense, because even if you select some random element within the bar, the frame isn't going to literally be inserted right there - it's still going to be inserted in front of the bar as a whole. And the palette at least does continue to require that, even if the Add menu does not. So I'd think of the occasional ability to select a random element within a bar as more of a bone to people with sloppy fingers than as something we want to encourage.

However, there is one element type that it is perhaps important to know you can select - another frame. So ideally, the message would say "select a bar or frame". And of course, it should actually say "measure" in the code, then the UK translation would change it to "bar".

MarcSabatella avatar Sep 19 '22 13:09 MarcSabatella

That makes sense to me @MarcSabatella.

So @wizofaus if I were to refine the specification of this fix, based on our discussion above, I would say:

  1. Just this one dialog for all text elements that currently trigger it (incl. figured bass): Screenshot 2022-09-19 at 5 55 56 pm

  2. And this dialog for frames: Screenshot 2022-09-19 at 5 56 28 pm

I would also add that the checkbox in both cases should be unchecked by default.

Does this all make sense?

bkunda avatar Sep 19 '22 15:09 bkunda

It makes sense to me. Most elements can be pretty cleanly separated into those that attach to individual notes/rests (most text, articulations, etc) and those that are added to measures or frames (most repeats, time signatures, breaks, spacers, and other measures and frames). The one slight outlier here are the repeats (including DC/DS/etc, measure repeats, voltas, etc), which can't be attached to frames.

MarcSabatella avatar Sep 19 '22 17:09 MarcSabatella

I believe that will need to wait for 4.1 given the "string freeze". I think we can agree that this PR at least improves the current situation in 4.0.

wizofaus avatar Sep 19 '22 19:09 wizofaus

I believe that will need to wait for 4.1 given the "string freeze". I think we can agree that this PR at least improves the current situation in 4.0.

Yes I believe you're right here.

Is the UI tweak something you were able to do @wizofaus?

bkunda avatar Sep 20 '22 10:09 bkunda

I'm inclined to say we shouldn't worry too much about string freeze, since we already have a bunch of changes/additions that are waiting to be pushed anyway. Since there's still enough time until the release, I think a few strings less or more doesn't make a huge difference.

cbjeukendrup avatar Sep 20 '22 10:09 cbjeukendrup

I'm inclined to say we shouldn't worry too much about string freeze,

Exactly. We did mention that there would be a few changes - but we'd keep them to a minimum. We generally agreed it was best to initiate translations rather than wait for the last few stragglers to make their way in.

Tantacrul avatar Sep 20 '22 11:09 Tantacrul

I'm inclined to say we shouldn't worry too much about string freeze,

Exactly. We did mention that there would be a few changes - but we'd keep them to a minimum. We generally agreed it was best to initiate translations rather than wait for the last few stragglers to make their way in.

Ok well let's push this forward then :-)

@wizofaus please let me know if/when you can implement the tweaks we've specified here. I'll give it another test before recommending it for merging.

bkunda avatar Sep 20 '22 11:09 bkunda