Cleaning C side of Shoes

[IanTrudel]

Shoes needs some refactoring and simplification in order to make it easier to understand and maintain. This issue is meant as notes for a future release.

We should enforce a coding style on every C and Objective-C files using Artistic Style (astyle). This will ensure style consistency in the code.

The stub.c is likely to be one of the reason antiviruses don't like Shoes. ShellExecute is generally not welcome.

It's hard to know where to start with this. It's not about rewriting the whole Shoes but we may consider restructuring, simplifying and splitting some code. Some code could be moved on Ruby side. Newest features like svg and plot have their own module, which is excellent.

There are oddities like struct definitions in the middle of app.c. There are also constant definitions a bit everywhere. There is dead code and commented code that would be best removed.

[ccoupe]

When you get a pile of C code handed to you to fix , you do not make it pass your style first before looking at it. _why liked opening '{' on separate lines. I prefer them at the end of the expression so I try to keep that _why standard. but nobody is consistent. I prefer putting {} around slngles line blocks in an if () { } when there is an else. I like tabs to be four spaces or two when the code is dense. Some folks actually indent to 5. Where did that come from? Ruby source likes 8 to a tab. Old battles, no winners. Leave it alone.

The stub.c is likely to be one of the reason antiviruses don't like Shoes. ShellExecute is generally not welcome

please send links to what should be done.

There are oddities like struct definitions in the middle of app.c. There are also constant definitions a bit everywhere.

structs are local to file they are defined and lexically visible after declaration - not for whole file. That's just C. ruby private if you want think that way although it's more subtle. Perfectly legal, practical and doesn't pollute the larger C name space - use extern for that.

[ccoupe]

The Obj-C style guides are are good looking tor documentation. Try writing code - unless you use xcode editor. Very confining an is anybody really going to care?

[IanTrudel]

_why has been long gone, abandoning ship. The thing is not even about who's coding style it is... the problem is inconsistency. It's messy. We could enforce YOUR preferences. It would be fine with me.

AStyle is vim compatible.

please send links to what should be done.

A bit ahead. I had tagged this issue for 3.3.4 to have more time to work on it. We still haven't shipped 3.3.3. :)

structs are local to file they are defined and lexically visible after declaration - not for whole file. That's just C. ruby private if you want think that way although it's more subtle. Perfectly legal, practical and doesn't pollute the larger C name space - use extern for that.

What you write does make sense. So if it is not part of the whole thing and doesn't belong to the top where the declarations are, wouldn't it belong to its own module? For example, I was thinking to add a GtkSwitch to Shoes — should it go in ruby.c, ruby.h, native.h, gtk.c, canvas.h and canvas.h... or... couldn't we simply have a widget/switch{.c, .h} ? (Shoes4 do something like that with all the widgets)

Shoes internal has a steep learning curve despite that the concept is actually rather simple.

Try writing code - unless you use xcode editor.

I'd say most people on MacOS X would use XCode. I apparently also use Xcode. haha

Very confining an is anybody really going to care?

Shoes is a mess. So every bit helps. Astyle is a tool already doing it for all platforms Shoes is available on. This thing can be automated anyway. Your coding style preferences.

[ccoupe]

I was thinking to add a GtkSwitch to Shoes — should it go in ruby.c, ruby.h, native.h, gtk.c, canvas.h and canvas.h.

You need Little bit's of code in all of the above because of the way C .h file and macros work. Other wise you'd have one giant file. When @passenger94 cleaned up the gtk3 converision. he took the route of creating many new shoes/native/gtk/ files

Shoes internal has a steep learning curve despite that the concept is actually rather simple.

Shoes -> layer of independent C (ruby.c, canvas.c, primarily, app and world for global things and startup) -> 3 platform GUI implementations. However those 3 (now two) bleed across all layers because C has limitations is abstraction - supposedly solved by C++ and then later by java to clean up that mess which has grown into (insert your noun). Remember, Ruby also has include files and macro we need to mix in, It's a lot to learn. I don't think anyone here understands it all, myself included.

[IanTrudel]

You need Little bit's of code in all of the above because of the way C .h file and macros work.

Yes, exactly my point. Adding simple things involve several files.

RE: abstraction

It's not necessary. It should be possible to segment code that will be compiled into object files (.o) and the linker will resolve references.

I don't think anyone here understands it all, myself included.

Words of wisdom.

[ccoupe]

We've add three widgets (svg, restored the vidoe, and plot). Figuring out where to put things was not particularly difficult - the compiler tells what you what you didn't do correctly. Minimal effort compared to writing the code to do it what ever it does. (cross platform --new widgets need a Cocoa implementation too).

The internals of Shoes 4 is not a quick learn either and it's best practices. There hasen't been a swarm of new contributers to their efforts either. Why? Perhaps there is a limited number of people who dig deep and learn complex things? Are they writing code for their phones/tablets or cloud applications and they don't want to deal with ancient languages like C where their skills are really challenged because C doesn't work the way they are expecting. IMHO, the money, glory, fame and fortune are with iOS and Android, java and the 'cloud' - all deep dives.

[ccoupe]

Adding simple things involve several files.

Because they aren't simple in the next layer down. Have you ever replace a garbage disposal in your sink? Looks easy. It almost is, but only after you learn some things by failing

[IanTrudel]

We don't have to strive to be Shoes4. Can't hurt to be inspired by them either.

Have faith in me. This is not the first time I suggest things for Shoes that has benefited Shoes. :)

I'm pretty sure I can present to you some proof of concept.

[IanTrudel]

Proof-of-concept you say? I managed to isolate slider and move it to shoes/widget/slider{.c,.h}. It's working as it should but it's also clear Ruby/Canvas need some kind of public interface to make it easier. Also would need a macro to call widget init function (shoes_<widget>_init()) and perhaps a Raketask to create a header with all widgets headers to be included into ruby.c.

Canvas related modifications:

• Remove VALUE shoes_canvas_slider(int, VALUE *, VALUE); in canvas.h
• Remove static from shoes_add_ele in canvas.c
• Remove shoes_canvas_slider in canvas.c

Ruby related modifications:

• Remove cSlider in extern VALUE declaration in ruby.h
• Remove f("+slider", slider, -1); \ in ruby.h
• Move RUBY_M, FUNC_M, SETUP_CONTROL, FINISH from ruby.c to ruby.h
• Remove static from call_cfunc in ruby.c
• VALUE call_cfunc(HOOK func, VALUE recv, int len, int argc, VALUE *argv); in ruby.h
• Remove shoes_slider_draw, shoes_slider_get_fraction and shoes_slider_set_fraction in ruby.c
• Remove Slider class definition in ruby.c (and all that goes with it), replace with shoes_slider_init()

Add "shoes/widget/*.c" to file_list in your env.rb.

shoes/video/video.c will not need to define call_cfunc anymore, as a bonus. (Hello rendundancy!)

shoes/widget/slider.h

#include "shoes/ruby.h"
#include "shoes/canvas.h"
#include "shoes/app.h"
#include "shoes/internal.h"
#include "shoes/world.h"
#include "shoes/native.h"

#ifndef SLIDER_H
#define SLIDER_H

/* extern variables necessary to communicate with other parts of Shoes */
extern VALUE cShoes, cApp, cTypes, cCanvas, cWidget;
extern shoes_app _shoes_app;

/* Should be automatically available but ruby.c is not sharing enough information */
extern VALUE shoes_control_change(int argc, VALUE *argv, VALUE self);

/* each widget should have its own init function */
void shoes_slider_init();

// ruby
VALUE shoes_slider_draw(VALUE self, VALUE c, VALUE actual);
VALUE shoes_slider_get_fraction(VALUE self);
VALUE shoes_slider_set_fraction(VALUE self, VALUE _perc);

// canvas
VALUE shoes_canvas_slider(int, VALUE *, VALUE);

// from canvas.c, should be moved in canvas.h
#define SETUP_CANVAS() \
  shoes_canvas *canvas; \
  cairo_t *cr; \
  Data_Get_Struct(self, shoes_canvas, canvas); \
  cr = CCR(canvas)

#endif

shoes/widget/slider.c

#include "shoes/widget/slider.h"

// ruby
VALUE cSlider;

FUNC_M("+slider", slider, -1);

void shoes_slider_init() {
   cSlider  = rb_define_class_under(cTypes, "Slider", cNative);
   rb_define_method(cSlider, "draw", CASTHOOK(shoes_slider_draw), 2);
   rb_define_method(cSlider, "fraction", CASTHOOK(shoes_slider_get_fraction), 0);
   rb_define_method(cSlider, "fraction=", CASTHOOK(shoes_slider_set_fraction), 1);
   rb_define_method(cSlider, "change", CASTHOOK(shoes_control_change), -1);
   
   RUBY_M("+slider", slider, -1);
}
  
VALUE shoes_slider_draw(VALUE self, VALUE c, VALUE actual) {
   SETUP_CONTROL(0, 0, FALSE);

   if (RTEST(actual)) {
      if (self_t->ref == NULL) {
         self_t->ref = shoes_native_slider(self, canvas, &place, self_t->attr, msg);
         shoes_control_check_styles(self_t);
         if (RTEST(ATTR(self_t->attr, fraction))) shoes_native_slider_set_fraction(self_t->ref, NUM2DBL(ATTR(self_t->attr, fraction)));
         shoes_native_control_position(self_t->ref, &self_t->place, self, canvas, &place);
      } else
         shoes_native_control_repaint(self_t->ref, &self_t->place, canvas, &place);
   }

   FINISH();
   
   return self;
}

VALUE shoes_slider_get_fraction(VALUE self) {
   double perc = 0.;
   GET_STRUCT(control, self_t);
   if (self_t->ref != NULL)
      perc = shoes_native_slider_get_fraction(self_t->ref);
   
   return rb_float_new(perc);
}

VALUE shoes_slider_set_fraction(VALUE self, VALUE _perc) {
   double perc = min(max(NUM2DBL(_perc), 0.0), 1.0);
   
   GET_STRUCT(control, self_t);
   if (self_t->ref != NULL)
      shoes_native_slider_set_fraction(self_t->ref, perc);

   return self;
}

// canvas
VALUE shoes_canvas_slider(int argc, VALUE *argv, VALUE self) {
   rb_arg_list args;
   VALUE slider;
   
   SETUP_CANVAS();

   rb_parse_args(argc, argv, "|h&", &args);

   if (!NIL_P(args.a[1]))
      ATTRSET(args.a[0], change, args.a[1]);

   slider = shoes_control_new(cSlider, args.a[0], self);
   shoes_add_ele(canvas, slider);
   
   return slider;
}

[ccoupe]

Don't forget to put the cocoa code in there.

[IanTrudel]

Uh? This proof-of-concept works with the inner layer of Shoes. It deals with Canvas and Ruby parts only. Gtk and Cocoa code absolutely untouched. Should work on MacOS X.

[ccoupe]

My mistake - I thought you were add a new widget, not moving one around. I don't see much of simplification gain myself and a whole lot of work.

[IanTrudel]

The problem is we don't know for sure what is implied before moving one around. Remember, this comes from an idea without any certainty it would work without a major rewrite.

This approach also need your feedback. It would use of a second pair of experienced eyes.

Simplifications?

• Slider widget is no longer all over the place. It now lives in its own module.
• Less clutter in Canvas and Ruby.
• Both will become better API. Easier to understand Canvas and Ruby core features.
• Debugging a widget will be easier.
• Migrating all existing widgets isn't particularly difficult.
• Widgets are a couple modifications away from being Plug'n'play. Adding a new widget in the future would no longer require changing other files.

Not sure why you are not enthusiastic. It's at the very least interesting findings.

Now that what needs to be proven is proven, it is possible to push the experiment further and implement a switch widget.

I'm working with the broken MSYS2 target. Let's see how it goes.

[ccoupe]

I'm not opposed if someone else does the work.
I think I've got msys2 working 'my way' - I have a ruby, but I'm updating the wiki and that takes twice (thrice) the time. Writing the documentation before beginning a large experiment is a useful way to get one's thoughts in order.

Please create a branch for your experiments.

[IanTrudel]

I'm not opposed if someone else does the work.

Proof-of-concept and prototypes are my bread and butter. So, I get it, you're afraid to be stuck with the concept but no substance.

You already know this is the way to go because you implemented svg and plot as module ... But still couldn't fully remove the strong ties to Ruby and Canvas.

You have in fact now all you need for them to be truly module based on my work.

Having common objectives. Aren't we a team? Sounds like a team...

MSYS2 target will it be fixed? This would be a good news.

[IanTrudel]

Please create a branch for your experiments.

Thanks, absolutely. Need to figure out how to do it first. :)

[IanTrudel]

Please create a branch for your experiments.

https://github.com/BackOrder/shoes3/tree/refactoring

[IanTrudel]

Most of them are relatively easy to move out of Ruby and Canvas. Some of them are a bit challenging, such as Progress.

Ran AStyle on all the new files. Should be to your liking. AStyle --style=java --indent-switches --indent-cases filename.c

What is TextEditBox? Seems different from EditBox. It's also undocumented.

[ccoupe]

TextEditBox is a future feature - there's an issue describing it and some thoughts.

[IanTrudel]

There is a new Switch widget in https://github.com/BackOrder/shoes3/tree/refactoring.

Sample code

Shoes.app do
   flow do
      switch; para
   end
   
   flow do
      @n = switch(active: true) do
         @p.text = @n.active? unless @p.nil?
      end
      @p = para
   end
   
   flow do
      @m = switch width: 80
      @m.click do
        @m.active? ? @e.start : @e.stop
      end
      @e = every(1) { |count| @q.text = count unless @q.nil? }
      @q = para
   end
   
   start do
      @e.stop
      @p.text = @n.active?
      @m.active = true
   end
end

[ccoupe]

Is it possible to change the on/off captions/labels that are displayed in the widget. More importanly, what's your assessment of whether moving all that code around leads to a better understanding of where to look for bugs - hard to assess, i know).

[IanTrudel]

I'm sorry about that. Refactoring Shoes comes with a price because Shoes is mostly monolithic. I had to start over several times while refactoring code. It is challenging now but it should make your life easier later.

[ccoupe]

It is challenging now but it should make your life easier later.

So you say. My life was fine. Your life might be easier. Do you happen to know a John Leake (perhaps in the UK but I suspect not, email header don't support that claim). I wasted a lot of time with that person and he's some sort of concern troll claiming he's 'very experienced' but can't be bothered to answer simple questions.

shoes/native/cocoa.m:388:3: warning: implicit declaration of function
      'shoes_button_send_click' is invalid in C99
      [-Wimplicit-function-declaration]
  shoes_button_send_click(object);
  ^
shoes/native/cocoa.m:646:3: warning: implicit declaration of function
      'shoes_timer_call' is invalid in C99 [-Wimplicit-function-declaration]
  shoes_timer_call(object);
  ^
shoes/native/cocoa.m:1511:3: warning: implicit declaration of function
      'shoes_native_edit_box_set_text' is invalid in C99
      [-Wimplicit-function-declaration]
  shoes_native_edit_box_set_text((NSControl *)tv, msg);
  ^
shoes/native/cocoa.m:1527:1: error: conflicting types for
      'shoes_native_edit_box_set_text'
shoes_native_edit_box_set_text(SHOES_CONTROL_REF ref, char *msg)
^
shoes/native/cocoa.m:1511:3: note: previous implicit declaration is here
  shoes_native_edit_box_set_text((NSControl *)tv, msg);
  ^
shoes/native/cocoa.m:1746:54: error: use of undeclared identifier 'cTimer'; did
      you mean 'timer'?
    andObject: self repeats:(rb_obj_is_kind_of(self, cTimer) ? NO : YES)];
                                                     ^~~~~~
                                                     timer
shoes/native/cocoa.m:1745:15: note: 'timer' declared here
  ShoesTimer *timer = [[ShoesTimer alloc] initWithTimeInterval: interval * 0.001
              ^
3 warnings and 2 errors generated.

[IanTrudel]

So you say. My life was fine. Your life might be easier.

I spent a month on refactoring Shoes. It did worth every bit of efforts. Now you don't have to spend that amount of time on fixing errors, those you shown above are related to native.h. You may have to include some types or all types/types.h in cocoa.m.

Do you happen to know a John Leake (perhaps in the UK but I suspect not, email header don't support that claim). I wasted a lot of time with that person and he's some sort of concern troll claiming he's 'very experienced' but can't be bothered to answer simple questions.

Whaaat?!

[ccoupe]

Along will all the new widgets and method needing implementation in Cocoa these look problematic

Undefined symbols for architecture x86_64:
  "_shoes_control_active", referenced from:
      _shoes_switch_init in switch.o
  "_shoes_native_control_get_tooltip", referenced from:
      _shoes_control_get_tooltip in native.o
  "_shoes_native_control_set_tooltip", referenced from:
      _shoes_control_set_tooltip in native.o

[ccoupe]

I spent a month on refactoring Shoes. It did worth every bit of efforts. Now you don't have to spend that amount of time on fixing errors

I do appreciate the effort, believe me. It will make fixing the all the Get_Struct changes we need to make to get to Ruby 2.3 become easier. Even better if when the rake files only compile changed .c/.h files.

[IanTrudel]

Thanks. Refactoring Rakefiles is also giving me a hard time. It's gonna take some time. I'm doing my best to avoid breaking builds but some is to be expected.

MacOS X may have some different expectations when it comes to linking. My understanding from the last error you report is it needs function prototypes from canvas. I normally don't include canvas in types, just a very few necessary prototypes.

[ccoupe]

OSX is compiling and linking with lots #ifdef GTK around the Ruby tooltips methods definitions. I don't know yet how to implement tooltips and callbacks in Cocoa for every Shoes widget.

It Runs. Sadly It doesn't have any text in the buttons - but that's probably something silly on my part.

[ccoupe]

but that's probably something silly on my part.

Not so. shoes_native_button is unchanged in cocoa.m and lldb says the msg parameter points to "\b". Linux gdb has the correct value "Shoes Info". I'm confused. How could that happen the stack backtrace looks reasonable.

[IanTrudel]

This is strange considering button is one of the simple widgets around. The noticeable difference is that SETUP_CONTROL used to be defined in ruby.c but is now in ruby.h. This is where the text attribute is used to fill msg parameter.

[ccoupe]

Found it. For shoes_native_button you added the attr parameter (to be consistent or tooltips or ..). Because I didn't include the shoes/types/type.h file properly, gcc didn't flag the signature change. So, it was my problem.

[ccoupe]

While we are cleaning up I saw that in gtk.c shoes_open_browser won't work on Linux (or windows) . It doesn't seem to have any Shoes/ruby level api either. Delete or fix?

[IanTrudel]

What does it do? I was conservative with the changes. Tested features regularly. Most likely moves elsewhere.

[ccoupe]

What does it do?

It open the given url in a browers - on osx it does an backticked open #{url} Remember the thing that was broken in the manual that we wrote our own function for -- we could have fixed and documented this one. I'm leaning towards deleting it.

I'm struggling with the switch widget. Why is the following line in shoes/native/gtk/gtkswitch.h

// EVENT_COMMON generated functions
VALUE shoes_control_active(int argc, VALUE *argv, VALUE self);

Should that be in shoes/native/switch.c (where cocoa/clang can find it). Actually cocoa sees the extern , it doesn't have the matching implementation. EVENT_COMMON(control, control, active) perhaps?

[IanTrudel]

It open the given url in a browers - on osx it does an backticked open #{url} Remember he thing that was broken in the manual that we wrote our of function for -- we could have fixed and documented this one. I'm learning towards deleting it.

So we already have a replacement? Let's do as you deem best.

Should that be in shoes/native/switch.c (where cocoa/clang can find it)?

Actually, it should be in shoes/types/switch.c rather than native. Move EVENT_COMMON(control, control, active) just below FUNC_M("+switch", switch, -1);. Also, it would no longer need shoes_control_active function prototype or at least no more extern in shoes/types/switch.h.

[ccoupe]

Thank you. Now compiling and linking for osx and linux. The widget code on OSX is stubbed out until I find a similar cocoa implementation on the interwebs. NSToggleButton or similar.