runModalLoop returning immediately


#1

I have just spent all day converting to the latest release from the early juce 2.0.

runModalLoop is returning immediately. Anybody got any ideas why?


#2

Although it’s very bad practice to use it, I’m pretty sure the method works! For example, it’s used by AlertWindow, and I know that works ok. Must be something you’re doing to cancel it, but I can’t think what.


#3

I found the problem and I really wish you would reconsider how freely you change juce. I have to edit almost 200 lines of code because of changes from 2.0 to the latest version. You changed Label::setText to use NotificationType as a second parameter, but did not do so for ComboBox::setText. So I have to make at least 50 changes to Label::setText. During the changes I also accidentally replace ComboBox::setText second parameter with dontSendNotification just like the Label:setText. Well these parameters are now opposite so the ComboBox was sending a notification at initialization which in my code triggered a button down in my code which exited the modalLoop. At least make all calls like this consistent.

Making changes like this is dangerous. Microsoft did a study a while back, and every 100 lines of code or so you type or change, you’re likely to enter a typing error or a copy/paste gotcha. Forcing customers to make changes like this is dangerous and is not a good practice. As you know and complain plenty about, Apple is famous for making random changes which causes plenty of headaches. It is because of Apple I am using juce. MFC by Microsoft definitely has it issues, but in 20 years of using MFC I never had to make changes like this. If it worked in 2008 the same code worked in 2012. That is good management and decision making. Please consider this. This is my third Juce change in a year and each time it takes a day or two to recover. I have a very big complicated program.

Finally, My advise is to add a different call is you want different functionality from an existing call. If it isn’t broken don’t fix it.

Example: I had to make 20 changes to Change Graphics::drawBevel to LookAndFeel:drawBevel. Why not just have the Graphics::drawBevel routine call the LookAndFeel drawBevel like the other routines. This way customers don’t have to make code changes and juce still adds functionality.

BTW, I am on my second day of converting to the latest juce.


#4

Well, I don’t make these decisions lightly, and don’t make changes unless there are good reasons…

Obviously with any change, I have to weigh its benefits against the annoyance of updating existing code (including my own annoyance, as I also have a whole bunch of projects that I need to keep updated!). But everyone has a different tolerance of how much change they’re willing to put up with in order to have a better product. Your opinion is noted, thanks!


#5

I hear you, but at least be consistent so that problems like above do not occur.

One more thing I can’t figure out how to do.

How do I add a DropShadow to a CallOutBox? My code worked in 2.0 but doesn’t work in the new juce. (Code please)

I previously did it in the LookAndFeel::drawCallOutBoxBackground but now it doesn’t work.


#6

You can copy the shadowing code from the LookAndFeel base class:

[code]void LookAndFeel::drawCallOutBoxBackground (CallOutBox& box, Graphics& g,
const Path& path, Image& cachedImage)
{
if (cachedImage.isNull())
{
cachedImage = Image (Image::ARGB, box.getWidth(), box.getHeight(), true);
Graphics g2 (cachedImage);

    DropShadow (Colours::black.withAlpha (0.7f), 8, Point<int> (0, 2)).drawForPath (g2, path);
}

…[/code]


#7

This does not work for me:

BTW, I have a transparent background set for the CallOutBox.

Here is my code:

[code]drawCallOutBoxBackground( CallOutBox &box, Graphics &g, const Path &path, Image &cachedImage )
{
g.setColour( gMainFillColour.withAlpha( .75f ) );

g.fillPath( path );

g.setColour( Colours::white.withAlpha (0.8f) );
g.strokePath( path, PathStrokeType (2.0f) );

if (cachedImage.isNull())
{
    cachedImage = Image (Image::ARGB, box.getWidth(), box.getHeight(), true);
    Graphics g2 (cachedImage);
    DropShadow (Colours::black.withAlpha (0.7f), 8, Point<int> (0, 2)).drawForPath (g2, path);
}

}[/code]


#8

BTW, the documentation is incorrect on CallOutBox under detailed description.

void mouseUp (const MouseEvent& e) { MyContentComponent content; content.setSize (300, 300); CallOutBox callOut (content, *this, nullptr); callOut.runModalLoop(); }

This was such a nice way of doint it. Why did you change it? Having a CallOutBox point to another component by passing in the Component was very intuitive. Now, again, I have to rewrite my code for this change. Why couldn’t you just add another constructor?


#9

Well yeah, obviously if you draw into an image and then do nothing with that image, then nothing will happen, right…? I only posted that code because you asked how to draw a drop-shadow, I expected you to actually think about it, not just paste it blindly into your code!

I changed the CallOutBox constructor for several reasons: there was definitely some kind of subtle problem with the old version which made it problematic, though I can’t remember what… But the main thing is that I changed the whole way that you’re supposed to use that class: Because modal loops are evil and must never be used, I added the launchAsynchronously() method, which you should always use instead of constructing one directly. So I figured I might as well improve the parameters, and deliberately allowed this to break old code, since anyone who was running these things modally using the old constructor really should change to running them asynchronously anyway. Sorry, I obviously forgot to update the docs to explain that, but have tweaked them now, thanks for the heads-up.


#10

Back to the bug you encountered, you accidentally changed some methods that don’t use the newer notification enum. And then - your compiler silently cast that the enum to a bool for you?

Is there a compiler setting that should catch that? Obviously, the other way around was caught (bool to enum), since you had to change. Shouldn’t enum > bool be flagged as an error?

I rather think that a compiler should help trap bugs, not API stagnation. The MFC point seems moot, since well, MFC sucks. Personally, considering my time investment in the codebase, I want Juce to keep improving, adding new features and safer methods to avoid new bugs in new code. If that means there’s a little time every now and again: so be it.

Bruce


#11

Agreed, one of the things I like so much about JUCE is that it is ever evolving and improving. We get to actively see API and style improvements such as replacing bool arguments with explicit enums. This makes code so much easier to read and after seeing the improvement in JUCE I now follow suit as much as possible. I would much rather prefer a ‘correct’ library rather than one that stagnates and improvements aren’t made where possible.

I also like the fact that old methods aren’t left in which forces us to update to the new versions and avoids confusion for duplicate methods. I would agree that it is somewhat inconvenient to update old code but every time a change is made that forces me to do so I prefer the result. Jules doesn’t however make code-breaking changes like this very often. I try to update JUCE versions in my projects about every week or so which reduces the amount of changes that need to be made so I can usually get them done in about 5-15 minutes just a couple of times a year.

Doing these changes as they arise instead of letting them accumulate also means that I’m more aware of each change so when I come to an older project that hasn’t been updated in a while I can spot what change each compiler error relates to.


#12

I would love to finally see ComboBox upgraded to use enums. It is the only place where you have to pass FALSE when you DO want a notification. Wart! :slight_smile:


#13

Yes, and I think the majority of people also want these improvements, but I’ve been doing them slowly to try to avoid getting moaned at! But good call - I’ll change ComboBox now, and if anyone gripes, I’ll blame you for requesting it!


#14

Actually, an interesting observation:

I’ve just spent a couple of minutes updating ComboBox to use the enum, and when I went through fixing the dozen or so places in my code where the old boolean parameter was used, I spotted 5 places in tracktion where the bool had been used the wrong way around. And at least one of those must have been causing an actual bug, so this kind of forced updating can actually have benefits!


#15

I agree that changes need to be made, and I am glad to see others benefited from my complaining.

Also this probably could have been avoided by making the second parameter default to dontSend, which I assume is the majority.

I’ll take the blame.


#16

As for the DropShadow change. The last parameter was recently added and because there is no documentation on how to use it I assumed the caller was going to draw it.

I also assumed this because,

DropShadow( Colours::black.withAlpha( 0.7f ), 5, CPoint( 2, 3 ) ).drawForPath( g, path );

fills the path with a dark background. I have a transparent background so this is not nice and I assume a bug. Shouldn’t drawForPath, just draw the path, not fill the path.

I was able to prevent this by using DropShadowEffect.


#17

I’m mostly against default parameter values in general, but particularly in cases like these. The only time that default values are ok is when their value is blindingly obvious, e.g. nullptrs, empty strings, etc. If there’s any chance that when reading or writing code that makes such a method call, you’d need to go and look-up the method signature to check what its default value is, then it definitely shouldn’t be defaulted, and should always be written explicitly.