Add option to disable the stitch plan cache

[george-steel]

Disabling the cache is necessary during development to ensure newly-changed code actually gets run.

Also fixes the error pane in the params gui.

[kaalleen]

Thanks for your pull request. I usually set the cache size to 0 for development, not sure if we need an extra option. What do you think @lexelby ?

I do like the rest of the changes. Especially making the display area for the param errors a little bit bigger really helps. I wanted to do this myself but haven't come around it ... Since we are on it ... Could you change

self.main_sizer.Add(self.warning, 1, wx.LEFT | wx.BOTTOM | wx.EXPAND, 10)

to

self.main_sizer.Add(self.warning, 0, wx.LEFT | wx.BOTTOM | wx.EXPAND, 10)

in warnings.py? No need for this text to take more space than necessary.

[george-steel]

Thanks for your pull request. I usually set the cache size to 0 for development, not sure if we need an extra option. What do you think @lexelby ?

I would say that just the cache size option makes things unclear and fragile, as any caching at all, even with a soft cap of zero (which can still leave an item in there) can interfere. The documentation is not exactly clear about how the cache works. I think that a definite off setting makes things easier as it avoids the need to know about the internals of the caching system to do anything else.

[kaalleen]

Don't get me wrong. I'm not opposed to the idea to add a disable option, I am just curious about opinions.

[lexelby]

I'm not opposed to the idea to add a disable option

I am. :) At least I lean toward opposing a disable option.

In my experience, users tend to disable caches when given the option and then forget about it. Then they get poor performance and blame the program. I don't want users to have to think about the cache, it should just do its job without problems. More options is not an improvement.

It's pretty unlikely that the cache will have anything in it when you set it to a size of 0. It's possible for the cull() call to time out, but it's unlikely. If you're really worried about that, just change it to pass retry=True.

[lexelby]

What changed in the preferences gui other than variable names? The constructor was generated using wxGlade, and if we change the code, it'll make it harder to make changes in Glade and regenerate.

[kaalleen]

The glade file can be found in the gui folder. It could be adapted as well. I do like more readable names.

[lexelby]

Sure, adjusting the glade file to match works for me.

[george-steel]

The constructor was generated using wxGlade, and if we change the code, it'll make it harder to make changes in Glade and regenerate.

It looks like the source file has already diverged a lot from the wxglade file before this PR. I'll look into bringing it back into sync, but it will probably expand the scope of this PR considerably.

[george-steel]

Currently (in main) the source and wxglade files have diverged in some way for every setting exposed in the dialog.

@lexelby @kaalleen Would it be better just to delete the wxg file instead of updating it? No other UI components currently use wxglade.

[kaalleen]

The only part that changed since introduction of the wxpython preferences is the addition of SetWindowStyle, so I'd be surprised if it changed that much. I think it would be enough to simply replace the variable names in the glade file.

[george-steel]

@Kalleen The code to load initial values of each input element also got inlined into widget-creation calls since the constructor was generated. Separating all of that out so that the constructor can be regenerated is a fairly major refactor,

[lexelby]

Sure, go ahead and get rid of the wxg file, I think I'm the only one that cared about it :)

[george-steel]

Sure, go ahead and get rid of the wxg file, I think I'm the only one that cared about it :)

Done. Is this ready to merge?

[kaalleen]

I think it is still in question if the disable stitch plan cache option should be introduced at all...

#2655 (comment)

[george-steel]

Disabling the stitch plan cache completely is a critical feature for debugging. If it truly needs to be hidden, an option would be to keep the cache-disabling logic but have it be triggered by a "magic" size value of 0 (which is otherwise a soft cap). I would prefer to have the explicit checkbox since that makes a lot more obvious what the option does.

[george-steel]

@lexelby What do you think?

[lexelby]

How about a setting to disable the cache in DEBUG.ini? Note that that's going to become DEBUG.toml as part of #2720 which should be ready pretty soon.

I do also think it's a good idea to pass retry=True to cull() just to be sure.

[george-steel]

How about a setting to disable the cache in DEBUG.ini? Note that that's going to become DEBUG.toml as part of #2720 which should be ready pretty soon.

@lexelby I'd prefer for the setting to be in the GUI, as it makes it easy to disable for development and re-enable once coding is complete for faster digitizing. Having it hidden in a config file makes it easy to forget that the cache is disabled.

[lexelby]

Alright, let's do your idea where setting it to 0 secretly actually fully disables the cache.

[george-steel]

Alright, let's do your idea where setting it to 0 secretly actually fully disables the cache.

@lexelby Done