serenity icon indicating copy to clipboard operation
serenity copied to clipboard

LibGUI: Introduce property deserializers to catch errors in GML property values

Open DanShaders opened this issue 2 years ago • 4 comments

Yes, I know, I am generally not really interested in Serenity-specific stuff. And yes, I know, I was porting DeprecatedString to StringBase. But suddenly task stack became really deep:

  • port DeprecatedString to StringBase
  • remove references to StringImpl
  • port JsonValue to Variant and StringBase
  • remove odd T JsonValue::to_something(T default_value) API
  • make LibGUI not use this API
  • do something so that GMLPlayground will not instantly crash on invalid input

Property deserializer is a small class which converts JsonValue to a required type while doing robust error checking at the same time. And previously that error checking in deserializing was basically "if something is supposed to be int but is not, a developer definitely wanted 0 there". What's more, if someone addresses one new error propagation FIXME I left, we will be able to show nice errors in GMLPlayground in real time.

I am aware of existence of GMLCompiler but AFAIC Film's intention is to continue using JSON input in GMLPlayground.

CC @kleinesfilmroellchen

Also, logging for discarded errors tells me that somewhere in DisplaySettings there is an invalid text_alignment value :)

DanShaders avatar Nov 06 '23 00:11 DanShaders

Also, logging for discarded errors tells me that somewhere in DisplaySettings there is an invalid text_alignment value :)

Never used gml/gui apps but maybe this is wrong?

MonitorSettings.gml:

text_alignment: "CenterMiddle"

Maybe it should be Center?

warpdesign avatar Nov 06 '23 08:11 warpdesign

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Nov 28 '23 02:11 stale[bot]

Not stale

DanShaders avatar Nov 28 '23 02:11 DanShaders

Now with 100% less short-lived macros!

DanShaders avatar Dec 09 '23 04:12 DanShaders

Sorry for taking so long on this! I like this change.

After testing it, I found a simple crash in GMLPlayground with invalid or in-progress property values.

For example:

@GUI::Widget {
   fixed_height: -1
}

will crash when typing the 1 with

125.446 GMLPlayground(44): VERIFICATION FAILED: value >= 0 at Userland/Libraries/LibGUI/UIDimensions.h:38
...
126.959 CrashReporter(45:46): 0x00000017a941bb0c: [/usr/lib/libc.so] ak_verification_failed +0x2c (Assertions.cpp:108)
126.959 CrashReporter(45:46): 0x00000017e7ba0ed9: [/usr/lib/libgui.so.serenity] AK::Function<bool (AK::JsonValue const&)>::CallableWrapper<void GUI::Object::register_property<GUI::Widget::Widget()::$_51, GUI::PropertyDeserializer<int>, GUI::Widget::Widget()::$_52>(AK::StringView, GUI::Widget::Widget()::$_51&&, GUI::PropertyDeserializer<int>&&, GUI::Widget::Widget()::$_52&&)::'lambda'(AK::JsonValue const&)>::call(AK::JsonValue const&) +0x59 (UIDimensions.h:38 => Widget.cpp:77 => Object.h:90 => Function.h:182)
126.959 CrashReporter(45:46): 0x00000017e7b01ab8: [/usr/lib/libgui.so.serenity] AK::Function<bool (AK::JsonValue const&)>::operator()(AK::JsonValue const&) const +0x38 (Function.h:115)
126.963 CrashReporter(45:46): 0x00000017e7b9cacf: [/usr/lib/libgui.so.serenity] GUI::Widget::load_from_gml_ast(AK::NonnullRefPtr<GUI::GML::Node const>, AK::ErrorOr<AK::NonnullRefPtr<Core::EventReceiver>, AK::Error> (*)(AK::StringView)) +0x35f (Widget.cpp:1101 => AST.h:186 => Widget.cpp:1100)
126.963 CrashReporter(45:46): 0x00000017e7b9d389: [/usr/lib/libgui.so.serenity] GUI::Widget::load_from_gml_ast(AK::NonnullRefPtr<GUI::GML::Node const>, AK::ErrorOr<AK::NonnullRefPtr<Core::EventReceiver>, AK::Error> (*)(AK::StringView)) +0xc19 (AST.h:222 => Widget.cpp:1131)
126.963 CrashReporter(45:46): 0x00000017e7b9c829: [/usr/lib/libgui.so.serenity] GUI::Widget::load_from_gml_ast(AK::NonnullRefPtr<GUI::GML::Node const>, AK::ErrorOr<AK::NonnullRefPtr<Core::EventReceiver>, AK::Error> (*)(AK::StringView)) +0xb9 (Widget.cpp:1095)
126.966 CrashReporter(45:46): 0x00000017e7b9c6a4: [/usr/lib/libgui.so.serenity] GUI::Widget::load_from_gml(AK::StringView, AK::ErrorOr<AK::NonnullRefPtr<Core::EventReceiver>, AK::Error> (*)(AK::StringView)) +0xa4 (Widget.cpp:1089)
126.966 CrashReporter(45:46): 0x0000000c92a757b7: [/bin/GMLPlayground] AK::Function<void ()>::CallableWrapper<MainWidget::try_create(GUI::Icon const&)::$_0>::call() +0x77 (MainWidget.cpp:93 => Function.h:182)
126.966 CrashReporter(45:46): 0x00000017e79f1dd3: [/usr/lib/libgui.so.serenity] AK::Function<void ()>::operator()() const +0x33 (Function.h:115)
126.966 CrashReporter(45:46): 0x00000017e7b55657: [/usr/lib/libgui.so.serenity] non-virtual thunk to GUI::TextEditor::document_did_change(GUI::AllowCallback) +0x17 (TextEditor.cpp:2282 => TextEditor.cpp:0)
126.966 CrashReporter(45:46): 0x00000017e7b3d8bb: [/usr/lib/libgui.so.serenity] GUI::TextDocument::update_views(AK::Badge<Syntax::TextDocumentLine>) +0x5b (TextDocument.cpp:173 => TextDocument.cpp:166)

... but seeing as that crash exists in master too, it seems like this is strictly an improvement :sweat_smile:

ADKaster avatar Jan 06 '24 11:01 ADKaster