Re: [Audacity-devel] seems strange to me -- so I made an effort to fix it

[Vaughan J. <va...@au...>]

Thanks, Gale. I had missed Darren's post. I'm not following all he's 
doing. A patch file would be better.
---

Gale (Audacity Team) wrote:
> It seems strange to me that if I try to save my project when it doesn't need to be saved, I get a message box telling me "Disallowed for some reason.  Try selecting some Audio first?".  Also, after that, the entire track is selected...
>
> So, I looked into this and came up with what I think is a reasonable work around.  I really think there would be a better way to handle this, but it would require a more invasive/dangerous approach and would require much more testing...
>
> In a nutshell, I changed HandleCommandEntry() from:
>
>      bool allowed = proj->TryToMakeActionAllowed( flags, entry->flags, 
> combinedMask );
>
>      if (!allowed)
>
> to this:
>
>       if(!((flags & combinedMask) == (entry->flags & combinedMask)))
>
> I robbed the condition from the TryToMakeActionAllowed() function and put it place of "allowed".
>   
I think the idea is roughly right, but would be better to change the 
return values from  TryToMakeActionAllowed rather than nearly duplicate 
some of its code. Certainly TryToMakeActionAllowed  isn't passing back 
some information it can find. Maybe just make TryToMakeActionAllowed 
give the messages.

---
> Then, in TellUserWhyDisallowed(), I added a bool and initialized it to false.
>
> bool reasonAccountedFor = false;
>
> If the reason is known, I change "reasonAccountedFor" to true and only display the wxMessageBox if "reasonAccountedFor" is true...
>   
But that means no alert when we can't figure out what's wrong -- the 
case that's most alarming.

---

> I tested the changes a little.  They seem to work and make Audacity work in a little more intuitive way.  But, before I put any more time into the solution, I thought I would run the whole idea past the main developers.
>
> As a side, I'm not a strong C++ programmer.  So, I welcome criticisms...
>
> Darren 
>
>
>
> Vaughan was a bit pessimistic it seems when thinking his changes to
> CommandManger.cpp as below might not fix all unwanted instances of 
> TellUserWhyDisallowed. It fixes all I know of on Windows and it's a 
> mighty relief. 
Thanks for checking. That's great news!

Not too pessimistic, just not optimistic enough. :-) I thought it would 
probably catch >50% of them. One of those bugs that took a couple of 
hours to find, another half hour to confirm (in my disbelief) and 30 
seconds to fix. It's been there a long time, definitely in 1.2.6.

---

> However it does kill the error message for edits on an 
> active track we previously thought worth giving:
>
> "You can only do this when playing and recording are stopped [Pausing 
> is not sufficient]". 
>
> I think this is probably OK (and far better than what we had) - for
> example we never threw an error for other disallowed actions when 
> recording like pressing "R". 
>   
+1. I say leave it this way.

---
> But I just wanted to draw attention to this thread again and Darren's 
> proposal in this context. 
I think we can augment TryToMakeActionAllowed  along those lines. I'm 
liking the idea of posting the alerts there, and I don't really see why 
TellUserWhyDisallowed is a separate method, since both of them are 
called in only one place, a few lines apart in 
CommandManager::HandleCommandEntry. And TellUserWhyDisallowed redoes 
some of the same checks that TryToMakeActionAllowed does. I think those 
two methods should be merged, and HandleCommandEntry simplified by a few 
lines.

---

> And I wondered if in fact we wanted to draw
> attention to a specific keyboard error in another way we could simply 
> produce a system ding like when you click outside a modal dialogue?
>   
Fair enough.

Thanks,
Vaughan
---
>
>
>
> Gale 
>  
>
>
> Index: CommandManager.cpp
> ===================================================================
> RCS file: /cvsroot/audacity/audacity-src/src/commands/CommandManager.cpp,v
> retrieving revision 1.61
> retrieving revision 1.62
> diff -u -d -r1.61 -r1.62
> --- CommandManager.cpp	23 Mar 2009 17:59:04 -0000	1.61
> +++ CommandManager.cpp	29 Mar 2009 02:57:20 -0000	1.62
> @@ -882,7 +882,7 @@
>  ///with the command's flags.
>  bool CommandManager::HandleCommandEntry(CommandListEntry * entry, wxUint32 flags, wxUint32 mask)
>  {
> -   if (!entry)
> +   if (!entry || !entry->enabled)
>        return false;
>  
>     wxUint32 combinedMask = (mask & entry->mask);
>
>
>
>
>
>
>