Adding any option on MenuItem fails

blueglyph commented 2 years ago

Description If I try to change the style of a menu item, or add a tooltip, the generated Rust code is incorrect.

Example of small UI, You can see the code in the repository https://github.com/blueglyph/test_fltk:

I set a label size of 12 for the two submenus File and Edit. The generated code becomes:

    pub fn make_window() -> Self {
    let mut my_win = Window::new(60, 729, 265, 190, "My Window");
    my_win.end();
    my_win.set_label_color(Color::by_index(32));
    my_win.make_resizable(true);
    my_win.show();
    let mut btn = Button::new(92, 80, 80, 30, "button");
    my_win.add(&btn);
    let mut fl2rust_widget_0 = MenuBar::new(10, 5, 240, 20, "App");
    fl2rust_widget_0.end();
    fl2rust_widget_0.set_frame(FrameType::FlatBox);
    fl2rust_widget_0.set_down_frame(FrameType::ThinUpFrame);
    my_win.add(&fl2rust_widget_0);

    File.set_label_size(12);  // <==== UNDEFINED "File"

    fl2rust_widget_0.add("File/Open", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("File/Save", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("File/Exit", Shortcut::None, MenuFlag::Normal, |_| {});
[...]

File is undefined, it's just the text of the submenu. It looks like {F&ile}.set_label_size if I use a shortcut symbol in the .fl file (see menu_font branch).

I tried to find a way to fix the code but I don't know FLTK enough, and the documentation requires some time to dig into - for example, the values returned by many methods like MenuBar::add are undocumented. Since the submenus are not added separately (for ex. File), I don't see yet how to access it and extract the reference to the item to set the size, tooltip and so on.

Code in repository In the repository, you can find the following:

the master branch is fine
the menu_font and menu_font2 branches expose the problem. They're attempts to change the label size in src/ui.fl, with the faulty result in src/ui.rs. A similar problem arises when trying to add a tooltip, and I assume most of the other options.

I'm using a local revision of fl2rust, hence my path reference in Cargo.toml.

Version fl2rust revision b30b4d727ec2f2fc68c0c3efdf3a89e59c255d09 ("add option to generate with directives preamble") toolchain 1.61.0

blueglyph commented 2 years ago

I found how to modify it, but the order of the instructions must change a little.

Instead of

    let mut fl2rust_widget_0 = MenuBar::new(10, 5, 240, 20, "App");
    fl2rust_widget_0.end();
    fl2rust_widget_0.set_frame(FrameType::FlatBox);
    fl2rust_widget_0.set_down_frame(FrameType::ThinUpFrame);
    my_win.add(&fl2rust_widget_0);
    File.set_label_size(12);
    fl2rust_widget_0.add("File/Open", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("File/Save", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("File/Exit", Shortcut::None, MenuFlag::Normal, |_| {});
    Edit.set_label_size(12);
    fl2rust_widget_0.add("Edit/Copy", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("Edit/Paste", Shortcut::None, MenuFlag::Normal, |_| {});

it should be

        let mut fl2rust_widget_0 = MenuBar::new(10, 5, 240, 20, "App");
        fl2rust_widget_0.end();
        fl2rust_widget_0.set_frame(FrameType::FlatBox);
        fl2rust_widget_0.set_down_frame(FrameType::ThinUpFrame);
        my_win.add(&fl2rust_widget_0);
        fl2rust_widget_0.add("File/Open", Shortcut::None, MenuFlag::Normal, |_| {});
        if let Some(mut item) = fl2rust_widget_0.find_item("File") {
            item.set_label_size(12);
        }
        fl2rust_widget_0.add("File/Save", Shortcut::None, MenuFlag::Normal, |_| {});
        fl2rust_widget_0.add("File/Exit", Shortcut::None, MenuFlag::Normal, |_| {});
        fl2rust_widget_0.add("Edit/Copy", Shortcut::None, MenuFlag::Normal, |_| {});
        if let Some(mut item) = fl2rust_widget_0.find_item("Edit") {
            item.set_label_size(12);
        }
        fl2rust_widget_0.add("Edit/Paste", Shortcut::None, MenuFlag::Normal, |_| {});

The order changes because at least one menu item must be added before doing the find_item.

This works with the '&' symbol used to add a menu shortcut:

        fl2rust_widget_0.add("F&ile/Open", Shortcut::None, MenuFlag::Normal, |_| {});
        if let Some(mut item) = fl2rust_widget_0.find_item("F&ile") {
            item.set_label_size(12);
        }

blueglyph commented 2 years ago

If I change the label size of a menu item, the problem is different. For example if I change File/Open of the above example, I get this code:

    pub fn make_window() -> Self {
    let mut my_win = Window::new(60, 729, 265, 190, "My Window");
    my_win.end();
    my_win.set_label_color(Color::by_index(32));
    my_win.make_resizable(true);
    my_win.show();
    let mut btn = Button::new(92, 80, 80, 30, "button");
    my_win.add(&btn);
    let mut fl2rust_widget_0 = MenuBar::new(10, 5, 240, 20, "App");
    fl2rust_widget_0.end();
    fl2rust_widget_0.set_frame(FrameType::FlatBox);
    fl2rust_widget_0.set_down_frame(FrameType::ThinUpFrame);
    my_win.add(&fl2rust_widget_0);

    fl2rust_widget_2.set_label_size(12);   // <==== UNDEFINED "fl2rust_widget_2"

    fl2rust_widget_0.add("File/Open", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("File/Save", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("File/Exit", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("Edit/Copy", Shortcut::None, MenuFlag::Normal, |_| {});
    fl2rust_widget_0.add("Edit/Paste", Shortcut::None, MenuFlag::Normal, |_| {});
    Self { my_win, btn, }
    }

So in this case, a "fl2rustwidget" is likely stored in the AST and retrieved by gen.rs, but the declaration is not written in the generated code.

MoAlyousef commented 2 years ago

So fltk-rs does menu's quite differently than FLTK. Actually this leads to much of the complexity in the parsing already, and also it means that not the same info can be passed to fltk-rs. The reason was that the way it's done in FLTK is quite difficult to translate to Rust. As such, I don't think this can be fixed without a lot of effort unfortunately.

My advice is to modify menu items in your source code, like you would add callbacks to your menu items.

blueglyph commented 2 years ago

Yes, I looked at the code and saw that it would require more than a small modification.

It's probably best to see the generated code as a starting point, then migrate it to the source code once the overall structure is right.

MoAlyousef commented 1 year ago

Hi This should be fixed with version 0.5.

blueglyph commented 1 year ago

I'm not sure. I've just updated the binary fl2rust from 0.4.17 to 0.5.6 and I can't parse the sample file I had anymore, unfortunately the last generated Rust file was not in version control so I lost it in the process.

ui.zip

d:\projects\rust\rpncalc\rpnc-gui\src>fl2rust thread 'main' panicked at 'index out of bounds: the len is 1 but the index is 1', %USERPROFILE%.cargo\registry\src\github.com-1ecc6299db9ec823\fl2rust-0.5.6\src\main.rs:12:33 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

d:\projects\rust\rpncalc\rpnc-gui\src>fl2rust ui.fl ui.rs thread 'main' panicked at 'called Result::unwrap() on an Err value: ParseIntError { kind: InvalidDigit }', %USERPROFILE%.cargo\registry\src\github.com-1ecc6299db9ec823\fluid-parser-0.1.7\src\parser.rs:193:90 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

d:\projects\rust\rpncalc\rpnc-gui\src>fl2rust ui.fl > ui.rs thread 'main' panicked at 'called Result::unwrap() on an Err value: ParseIntError { kind: InvalidDigit }', %USERPROFILE%.cargo\registry\src\github.com-1ecc6299db9ec823\fluid-parser-0.1.7\src\parser.rs:193:90 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

I've tried with another simple file and it didn't crash, though the result is empty:

// Automatically generated by fl2rust

#![allow(unused_variables)]
#![allow(unused_mut)]
#![allow(unused_imports)]
#![allow(dead_code)]
#![allow(clippy::needless_update)]

use fltk::browser::*;
use fltk::button::*;
use fltk::dialog::*;
use fltk::enums::*;
use fltk::frame::*;
use fltk::group::*;
use fltk::image::*;
use fltk::input::*;
use fltk::menu::*;
use fltk::misc::*;
use fltk::output::*;
use fltk::prelude::*;
use fltk::table::*;
use fltk::text::*;
use fltk::tree::*;
use fltk::valuator::*;
use fltk::widget::*;
use fltk::window::*;

main.zip

I'm currently working on something else so I'll need more time to understand this. I'll need to revert to the previous binary first and compare the results.

MoAlyousef commented 1 year ago

Hmm trying the first file with the latest fl2rust:

// Automatically generated by fl2rust

#![allow(unused_variables)]
#![allow(unused_mut)]
#![allow(unused_imports)]
#![allow(dead_code)]
#![allow(clippy::needless_update)]

use fltk::browser::*;
use fltk::button::*;
use fltk::dialog::*;
use fltk::enums::*;
use fltk::frame::*;
use fltk::group::*;
use fltk::image::*;
use fltk::input::*;
use fltk::menu::*;
use fltk::misc::*;
use fltk::output::*;
use fltk::prelude::*;
use fltk::table::*;
use fltk::text::*;
use fltk::tree::*;
use fltk::valuator::*;
use fltk::widget::*;
use fltk::window::*;

#[derive(Debug, Clone)]
pub struct UserInterface {
    pub win: Window,
    pub menu: MenuBar,
    pub g_display: Group,
    pub txt_display: TextDisplay,
    pub txt_status: Output,
    pub g_keyboard: Group,
}

impl UserInterface {
    pub fn make_window() -> Self {
        let mut win = Window::new(11, 215, 264, 550, None);
        win.set_label("RPN Calculator");
        win.end();
        win.set_type(WindowType::Double);
        win.make_resizable(true);
        win.show();
        win.set_label_color(Color::by_index(131));
        let mut menu = MenuBar::new(0, 5, 265, 20, None);
        menu.end();
        menu.set_frame(FrameType::FlatBox);
        menu.set_down_frame(FrameType::FlatBox);
        win.add(&menu);
        let idx = menu.add_choice("F&ile/Open");
        menu.at(idx).unwrap().set_shortcut(unsafe {std::mem::transmute(0x4006f)});
        let idx = menu.add_choice("F&ile/Save");
        menu.at(idx).unwrap().set_shortcut(unsafe {std::mem::transmute(0x40073)});
        let idx = menu.add_choice("F&ile/Exit");
        let idx = menu.add_choice("&Functions/Trig/cos");
        let idx = menu.add_choice("&Functions/Trig/sin");
        let idx = menu.add_choice("&Functions/Trig/tan");
        let idx = menu.add_choice("&Functions/Trig/atan");
        let idx = menu.add_choice("&Functions/Logic/not");
        let idx = menu.add_choice("&Functions/Logic/and");
        let idx = menu.add_choice("&Functions/Logic/or");
        let idx = menu.add_choice("&Functions/Logic/xor");
        let idx = menu.add_choice("&Functions/General/sqr");
        let idx = menu.add_choice("&Functions/General/sqrt");
        let idx = menu.add_choice("&Functions/General/pow");
        let mut g_display = Group::new(0, 30, 264, 170, None);
        g_display.end();
        win.resizable(&g_display);
        g_display.set_color(Color::by_index(7));
        win.add(&g_display);
        let mut txt_display = TextDisplay::new(0, 30, 264, 170, None);
        g_display.resizable(&txt_display);
        txt_display.set_frame(FrameType::UpFrame);
        txt_display.set_text_font(Font::by_index(4));
        g_display.add(&txt_display);
        let mut txt_status = Output::new(1, 201, 262, 23, None);
        txt_status.set_color(Color::by_index(23));
        txt_status.set_frame(FrameType::BorderBox);
        txt_status.set_text_font(Font::by_index(4));
        txt_status.set_text_size(12);
        g_display.add(&txt_status);
        let mut g_keyboard = Group::new(0, 225, 264, 325, None);
        g_keyboard.end();
        win.add(&g_keyboard);
        Self {
            win,
            menu,
            g_display,
            txt_display,
            txt_status,
            g_keyboard,
        }
    }
}

The second fluid file uses a widget_class which was never really supported by fl2rust. The problem is that it's difficult to support since it's not clear what it should generate. Maybe someone can propose something for a next release.

Changing it to use a class then function:

# data file for the Fltk User Interface Designer (fluid)
version 1.0400
header_name {.h}
code_name {.cxx}
class UserInterface {open
} {
  Function {make_window()} {open
  } {
    Fl_Window {} {open
      xywh {368 463 265 208} type Double visible
    } {
      Fl_Menu_Bar {} {
        label App open
        xywh {5 0 265 20}
      } {
        Submenu {} {
          label {F&ile} open
          xywh {0 0 62 20}
        } {
          MenuItem {} {
            label Open
            xywh {0 0 30 20}
          }
          MenuItem {} {
            label Save
            xywh {0 0 30 20}
          }
          MenuItem {} {
            label Exit
            xywh {0 0 30 20}
          }
        }
        Submenu {} {
          label {&Functions} open
          xywh {0 0 62 20}
        } {
          Submenu {} {
            label Trig open
            xywh {0 0 62 20}
          } {
            MenuItem {} {
              label cos
              xywh {0 0 30 20}
            }
            MenuItem {} {
              label sin
              xywh {0 0 30 20}
            }
            MenuItem {} {
              label tan
              xywh {0 0 30 20}
            }
            MenuItem {} {
              label atan
              xywh {0 0 30 20}
            }
          }
          Submenu {} {
            label Logic open
            xywh {5 5 62 20}
          } {
            MenuItem {} {
              label not
              xywh {0 0 30 20}
            }
            MenuItem {} {
              label and
              xywh {0 0 30 20}
            }
            MenuItem {} {
              label or
              xywh {0 0 30 20}
            }
            MenuItem {} {
              label xor
              xywh {0 0 30 20}
            }
          }
          Submenu {} {
            label General open
            xywh {0 0 62 20}
          } {
            MenuItem {} {
              label sqr
              xywh {0 0 30 20}
            }
            MenuItem {} {
              label sqrt
              xywh {0 0 30 20}
            }
            MenuItem {} {
              label pow
              xywh {0 0 30 20}
            }
          }
        }
      }
    }
  }
}

Generates:

// Automatically generated by fl2rust

#![allow(unused_variables)]
#![allow(unused_mut)]
#![allow(unused_imports)]
#![allow(dead_code)]
#![allow(clippy::needless_update)]

use fltk::browser::*;
use fltk::button::*;
use fltk::dialog::*;
use fltk::enums::*;
use fltk::frame::*;
use fltk::group::*;
use fltk::image::*;
use fltk::input::*;
use fltk::menu::*;
use fltk::misc::*;
use fltk::output::*;
use fltk::prelude::*;
use fltk::table::*;
use fltk::text::*;
use fltk::tree::*;
use fltk::valuator::*;
use fltk::widget::*;
use fltk::window::*;

#[derive(Debug, Clone)]
pub struct UserInterface {
}

impl UserInterface {
    pub fn make_window() -> Self {
        let mut fl2rust_widget_0 = Window::new(368, 463, 265, 208, None);
        fl2rust_widget_0.end();
        fl2rust_widget_0.set_type(WindowType::Double);
        fl2rust_widget_0.show();
        let mut fl2rust_widget_1 = MenuBar::new(5, 0, 265, 20, None);
        fl2rust_widget_1.set_label("App");
        fl2rust_widget_1.end();
        fl2rust_widget_0.add(&fl2rust_widget_1);
        let idx = fl2rust_widget_1.add_choice("F&ile/Open");
        let idx = fl2rust_widget_1.add_choice("F&ile/Save");
        let idx = fl2rust_widget_1.add_choice("F&ile/Exit");
        let idx = fl2rust_widget_1.add_choice("&Functions/Trig/cos");
        let idx = fl2rust_widget_1.add_choice("&Functions/Trig/sin");
        let idx = fl2rust_widget_1.add_choice("&Functions/Trig/tan");
        let idx = fl2rust_widget_1.add_choice("&Functions/Trig/atan");
        let idx = fl2rust_widget_1.add_choice("&Functions/Logic/not");
        let idx = fl2rust_widget_1.add_choice("&Functions/Logic/and");
        let idx = fl2rust_widget_1.add_choice("&Functions/Logic/or");
        let idx = fl2rust_widget_1.add_choice("&Functions/Logic/xor");
        let idx = fl2rust_widget_1.add_choice("&Functions/General/sqr");
        let idx = fl2rust_widget_1.add_choice("&Functions/General/sqrt");
        let idx = fl2rust_widget_1.add_choice("&Functions/General/pow");
        Self {
        }
    }
}

blueglyph commented 1 year ago

I have the same error using Arch, perhaps I'm doing something wrong.

updated fl2rust to 0.5.6
cd to where the ui.fl file is located
fl2rust ui.fl

$ fl2rust ui.fl thread 'main' panicked at 'called Result::unwrap() on an Err value: ParseIntError { kind: InvalidDigit }', /home/dev/.cargo/registry/src/github.com-1ecc6299db9ec823/fluid-parser-0.1.7/src/parser.rs:193:90 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

blueglyph commented 1 year ago

It doesn't help to have the unwraps without treating the errors in fluid-parser, but I found out why this was happening.

The "ui.fl" file has CR/LF EOL, if I convert it to Unix LF, it works correctly.

It happens near the end of the source file, line 105, because it's the only line ending with a parameter ("textsize 12").

blueglyph commented 1 year ago

I think this should solve the problem in fluid-parser, but I may have overlooked other things. The same error can occur with other trailing characters like \t.

--- a/src/lexer.rs
+++ b/src/lexer.rs
@@ -11,7 +11,7 @@ impl<'a> Lexer<'a> {
         let size = source.len();
         let mut cursor = 0;
         if source.starts_with("# ") {
-            while cursor < size && source.as_bytes()[cursor] != b'\n' {
+            while cursor < size && source.as_bytes()[cursor] != b'\n' && source.as_bytes()[cursor] != b'\r' {
                 cursor += 1;
             }
         }
@@ -31,7 +31,7 @@ impl<'a> Lexer<'a> {
             return t;
         }
         while self.cursor < self.size
-            && (self.s.as_bytes()[self.cursor] == b' ' || self.s.as_bytes()[self.cursor] == b'\n')
+            && self.s.as_bytes()[self.cursor].is_ascii_whitespace()
         {
             self.cursor += 1;
         }
@@ -61,8 +61,7 @@ impl<'a> Lexer<'a> {

                 t.start = self.cursor;
                 while self.cursor < self.size
-                    && self.s.as_bytes()[self.cursor] != b' '
-                    && self.s.as_bytes()[self.cursor] != b'\n'
+                    && !self.s.as_bytes()[self.cursor].is_ascii_whitespace()
                     && self.s.as_bytes()[self.cursor] != b'}'
                 {
                     self.cursor += 1;

I would definitely add unit tests and perhaps do something about the unwraps. There are many of them so a simple function could conveniently tell the user at least which line in the source file caused the panic.

blueglyph commented 1 year ago

EDIT: just added another modification in the patch above for line 11, for the sake of completeness. 🙂

Do you prefer if I submit a pull request?

MoAlyousef commented 1 year ago

I was just working on this and pushed my improvements. CRLF now is supported, as well as better error messages

blueglyph commented 1 year ago

Ah, OK. I was quickly doing the same by adding a simple generic function and a line field in the tokens, but now it's perhaps too late.

For instance with this Parser method:

    fn parse_arg<T>(&mut self) -> T where T: FromStr {
        let token = self.tokens[self.i];
        match T::from_str(token.word) {
            Ok(value) => value,
            Err(_) => {
                panic!("unexpected argument {} line {}", token.word, token.line)
            }
        }
    }

you can replace all the self.tokens[self.i].word.to_string().parse().unwrap_or_else... by self.parse_arg(), which may be easier to maintain and reduce the code size (replacing the panic message by yours).

blueglyph commented 1 year ago

Now backtracking a little... 😉

Now the generated Rust code is fine with the modifications that caused it to fail before (for ex. with labelsize).

Thanks!

fltk-rs / fl2rust

Adding any option on MenuItem fails #19