Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getters and Setters #13

Closed
JamesBremner opened this issue Jul 30, 2021 · 33 comments
Closed

Getters and Setters #13

JamesBremner opened this issue Jul 30, 2021 · 33 comments
Labels
question Further information is requested

Comments

@JamesBremner
Copy link

I am confused by the attribute getters and setters.

For example

https://alecmus.ml/lecui/html/classliblec_1_1lecui_1_1color.html

image

For the first, how can this be used to set the color?

This question applies to many of the attributes of other clesses too, which only have one getter/setter of this form.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

Say we have the following label and we would like to make it red and semi-transparent:

auto& label = lecui::widgets::label_builder(home)();
label
    .text("This is a simple <strong>label</strong>.")
    .rect().left(5.f);

We can do it using any of the following methods. Each is useful in different contexts. The different options are available so the coder can use whichever style they prefer, even though there is a recommended way.

// method 1 (recommended under most circumstances)
label.color_text().red(255).alpha(150);

// method 2 (recommended for chaining other label properties)
label.color_text(lecui::color().red(255).alpha(150));

// method 3
label.color_text() = lecui::color().red(255).alpha(150);

// method 4
label.color_text() = { 255, 0, 0, 150 };

// method 5
label.color_text({ 255, 0, 0, 150 });

// method 6
label.color_text().red() = 255;
label.color_text().alpha() = 150;

The result:

screenshot

For the first, how can this be used to set the color?

By modifying the reference returned by the .red() method as in method 6 above.

Here is the full code I have used for this example:

#include <liblec/lecui/containers/page.h>
#include <liblec/lecui/widgets/label.h>
using namespace liblec;

class sample_form : public lecui::form {
    lecui::page_manager _page_man{ *this };

    bool on_layout(std::string& error) override {
        auto& home = _page_man.add("home");

        // make a label
        auto& label = lecui::widgets::label_builder(home)();
        label
            .text("This is a simple <strong>label</strong>.")
            .rect().left(5.f);

        // method 6
        label.color_text().red() = 255;
        label.color_text().alpha() = 150;

        _page_man.show("home");
        return true;
    }

public:
    sample_form() :
        form("Sample Form") {}
};

int main() {
    sample_form form;
    std::string error;
    if (!form.show(error))
        form.message(error);
    return 0;
}

Note: the .get_red() (and similar getters) is strictly for reading and is required when extracting the color property from a const color object.

@alecmus alecmus added the question Further information is requested label Jul 30, 2021
@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

Method 2 is particularly useful for chaining as follows:

// make a label
auto& label = lecui::widgets::label_builder(home)();
label
    .text("This is a simple <strong>label</strong>.")
    .color_text(lecui::color().red(255).alpha(100))
    .rect().left(5.f);

This is because .color_text in this case returns a reference to the label for further modification of label properties (.rect() in this case). This avoids needlessly repeating explicit calls to the label reference.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

Methods 4 and 5 are generally not recommended because they're less readable. One would need to know the order of parameters in the color class constructor overload to understand what the values represent, but it is still added to accommodate coders that prefer this method for its brevity.

@JamesBremner
Copy link
Author

For the first, how can this be used to set the color?

By modifying the reference returned by the .red() method as in method 6 above.

This is very unusual! I have never seen this before. It would work, but it seems to me that it breaks the encapsulation of the attribute, which is considered a bad idea. If you are going to let the application code do this, whey not just make the attribute public? Then you can get rid of all the getters and setters.

@tmnyoni
Copy link
Contributor

tmnyoni commented Jul 30, 2021

Let's also consider that the library was designed, with (readability and less typing) when developing. So, then the getters and setters make that kind of implementation possible.

Maybe let's consider the following example:

example 1:

auto& label = lecui::widgest::label_builder(home)();
label.text("label");
label.color_text({255, 255, 255, 255});
label.rect().size({244, 20});
label.rect().set(0, 0, 244, 20);

example 2:

auto& label = lecui::widgets::label_builder(home)();
label
     .text("label")
     .color_text(lecui::color().red(255).green(255).blue(255))
     .rect().size(size().width(200.f).height(20.f))
     .set(rect().left(0).top(0).width(200.f).height(20.f));

I think from example 2 we can clearly see that you don't need to repeat the explicit calls to label as @alecmus mentioned and the code is readable.
And the use of getters enables the coder to do that chaining above.

@JamesBremner
Copy link
Author

I agree that having a setter the returns a reference to the object ( not the attribute ) is good because it allows chaining.

Having a getter that returns the attribute value ( not a reference to the attribute ) is good because it encapsulates the attribute and allows future development without breaking the class interface.

Having a method that returns a reference to an attribute is bad. For a great many classes in this library this bad technique is the only way to access the attributes.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

If you are going to let the application code do this, whey not just make the attribute public?

The attributes were made private to encourage the use of chaining, and once they are public then the methods would need to have different names, which leads to less readability and more typing. Emphasis is on readability and chaining.

Having a method that returns a reference to an attribute is bad.

Why really is returning a reference to an attribute bad when it serves exactly the same purpose as accessing the attribute directly? It would be bad for attributes that are not supposed to be modified, but in this case modifying those attributes is the whole point, but doing it in such a way that everything flows with consistent naming and minimal typing.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

The attributes were made private to encourage the use of chaining, for example, they were private in the color class until 9075042 but were made private thereafter as they needed to have names that are different from the chaining methods (e.g. _red). Since once there is an attributed named "red" there cannot be a method named "red", and we really need the interface to have methods that are named this way and not java-style "SetRed" or otherwise "set_red" etc. as this leads to a lot of unnecessary typing and more crowded code. Under very rare circumstances the .get_red() method is necessary when a const object is being used since even the compiler will not allow getting from a method that returns a reference, in which case when the coder tries to call .red() they get an error at compilation time or right from the visual IDE.

By using methods that are named exactly after the name of the attribute in question then it allows for code that is smoother to type and easier to the eye. And if one needs to modify only a single attribute then they can use the method that returns a reference.

This should not be an issue as long as the programmer follows the proper guidelines, e.g. making sure their application is compiled by the same compiler version as their copy of the gui library since they will be accessing STL objects across a DLL interface. It becomes a problem if the programmer breaks these rules.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

Since the library uses chaining, any problems that could arise from the return of references across a DLL interface would affect everything (chaining, attribute access through the references, etc.) not just this. The programmer should ensure they use the same compiler version under all circumstances and not blindly use the precompiled binaries that are currently built using Visual Studio 2019 as indicated in the download README. Furthermore, using references rather than copies leads to more efficient execution which is why we preferred it.

Please note that only attributes that should be modifiable are accessible this way.

@JamesBremner
Copy link
Author

JamesBremner commented Jul 30, 2021

Why really is returning a reference to an attribute bad

For a complete answer to this you should read a book on object orientated design.

Mostly the reasons have to do with the ease of future development. Here are the two principal reasons.

  1. Suppose you change the type of the attribute, say change it from integer to float. You will have to change all the application code that uses it. If you use a standard getters/setters, then you can change attributes in many ways without changing the class interface and the application code never needs to know so long as the bahvious is the same.
  2. Suppose that need to implement a side effect when the attribute value changes. This is easily implemented in a standard setter. Your setter that returns a reference to the attribute makes implementing side effects just about impossible.

Bottom line: implementing accessors that break the class encapsulation makes you code fragile, prone to bugs and very hard to develop any further.

@JamesBremner
Copy link
Author

Since the library uses chaining

As I said before, this is a good feature. The problem this issue is concerned with is the method that returns a reference to the attribute. Returning a reference to the object is fine.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

The problem this issue is concerned with is the method that returns a reference to the attribute. Returning a reference to the object is fine.

But both the object and the attribute are in the same memory space are they not? Both are created by code inside the DLL that is executed by the calling code at runtime. So if accessing the object through a reference is ok then why shouldn't it be ok to return a reference to the attribute as well since the attribute belongs to the object? It's contradictory.

From my understanding returning a reference is only a problem if the calling code and the DLL are compiled using different versions of the compiler or different compilers altogether. I don't see how it's a problem.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

If you use a standard getters/setters, then you can change attributes in many ways without changing the class interface and the application code never needs to know so long as the bahvious is the same.

I believe this is true only for attributes that should not be directly accessible to the caller. Here we're talking about attributes that need to be accessible. Say we revert back to 9075042 for the color class: the user would directly modify the attributes anyway, it's really the same thing.

class color {
public:
    float red;
}

color c;
c.red = 44;

is exactly the same to the compiler as this:

class color {
   float _red;
public:
    float& red() { return _red; }
}

color c;
c.red() = 44;

In both cases we access the attribute directly anyway.

The only difference between them is that in the second case we can add another method, also called red, that allows chaining as follows:

class color {
   float _red;
public:
    float& red() { return _red; }
   color& red(const float r) { _red = r; return *this; }
}

color c;
c.red() = 44;
c.red(23).red(25);

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

Suppose you change the type of the attribute, say change it from integer to float.

If we would need to do this after a stable release then that would be a serious design problem. We would need to release an entirely different version of the product for this, e.g. how Microsoft had to release Direct2D 1.1 because of changes that could not be incorporated into Direct2D 1.0.

As of now we're in alpha status ... still mapping the interface and so on. But once we get to beta and later make a stable release then it is not expected that we would change anything that is visible publicly in this way under any circumstances. The fact that it is visible publicly is because we don't expect such changes once we reach a stable build.

Imagine Microsoft needing to change the attributes in the RECT struct from INT to say FLOAT. It would be the same as us changing the _red from float to day double within the same major product version.

@JamesBremner
Copy link
Author

If we would need to do this after a stable release then that would be a serious design problem.

It is not a problem if the class interface has been correctly designed. This happens all the time. I have frequently done it myself and it only causes problems when the class encapsulation is broken.

class color {
public:
float red;
}

Do not make your attributes public. This also breaks encapsulation, and causes endless problems as you go forward.

@JamesBremner
Copy link
Author

Have you ever seriously studied object orientated design?

@JamesBremner
Copy link
Author

I believe this is true only for attributes that should not be directly accessible to the caller.

All attributes should be private, for the reasons I described above. Making them public breaks encapsulation and makes it very hard to further develop the class implementation.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

Do not make your attributes public. This also breaks encapsulation, and causes endless problems as you go forward.

I don't yet see the real argument behind this. And yes I have studied object oriented design and from my understanding this is an issue if the user uses a different compiler (as mentioned above).

And may I ask, what is the difference between these three:

class rect {
public:
   float left, right, top, bottom;
}

and this

struct rect {
   float left, right, top, bottom;
};

and this (from windef.h)

typedef struct tagRECT
{
    LONG    left;
    LONG    top;
    LONG    right;
    LONG    bottom;
} RECT, *PRECT, NEAR *NPRECT, FAR *LPRECT;

Are they not the same thing? They are! Microsoft made the .left, .top, etc attributes of the RECT struct public. What's wrong with that? They made them public because they don't expect them to change for a really long time and when the day comes that their data type needs to change then a different header will be used other than <Windows.h>. Contrary to what you're suggesting I really do understand how this works.

@JamesBremner
Copy link
Author

is exactly the same to the compiler as this:

The compiler will issue the same machine code, yes. We are not talking about writing machine code, we are talking about design and how to make it robust and future proof. You need to think about design, not write as if you were just using a compiler because it was a handy way to write machine code.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

Have you ever seriously studied object orientated design?

I would humbly suggest that you reconsider your perspective on why you think making references to attributes for objects like the ones we are discussing about is bad given this statement. Kindly reconsider all the facts.

Making a reference to a widget's implementation accessible is what would be really bad (e.g. label_impl in this case). Here we're dealing with structures that define parameters much like how we pass parameters to a function. This is my humble suggestion, instead of quickly assuming I don't understand it and concluding that the design is bad just because it is different to what you have seen. And I say that with respect and humility because I know you have a lot of experience in programming.

And I appreciate the feedback but on this particular matter please look at it carefully again.

@JamesBremner
Copy link
Author

Microsoft made the .left, .top, etc attributes of the RECT struct public. What's wrong with that?

The reason that we write gui framework libraries is because Microsoft code is so awful. If you were satisficed with the quality of the Microsoft Windows API, then you would use it directly.

@JamesBremner
Copy link
Author

Yes, I have a lot of experience in coding. I used to always declare the attributes public, because I found write accessors tedious. What's the harm, I thought.

Then some of my code became successful and clients started asking for enhancements. I had to add features. But all my attributes were public, so Had to

  1. Make the attributes private
  2. Write the accessors I should have done before
  3. Change all the application code to call the accessors wherever they were directly accessing the now private attributes

Believe me step 3 is really, really tedious.

Don't make the same mistake I made.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

I understand your concern, and it's really appreciated. But consider carefully the difference between these two classes:

The label_specs class in this file and the label_impl class here.

To the user of the library the label is actually label_specs, but please note that this is merely a struct. Modifying it's values does not modify the values of label_impl directly. For example, changing color_text in label_specs does not directly change the brush _p_brush (ID2D1SolidColorBrush* _p_brush) in label_impl. The brush is recreated internally by the library. The mechanics of this can be changed completely without having to change anything in the label_specs structure. The label_specs structure (which we are just about to rename to label since to the outside this is what appears to be the label) is really just a set of parameters that are used internally by the library to create the real objects. Perhaps if you take more time to look at the code as a whole you will see that I mean. The implementation is completely seperate from the interface in this library. We do not allow the direct modification of the internal objects. Please look more carefully as the interface does feel like you are modifying the actual objects but that is not the case.

Kindly note that label_impl is not even a member of label_specs. In other words label_specs is really not the label object even though that what we interact with from the outside. This was a very conscious design choice. The actual label is internal and the user has no way to access it, not even through setters and getters, unlike in other libraries.

The change of attributes is detected using a different mechanism. Kindly refer to label_impl.cpp to see what I mean. The actual widget resources are created in the create_resources method of label_impl, and only recreated if the label_specs changes. The process is de-coupled. Kindly look carefully, perhaps this might not turn out to be such a great concern.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

However let me add this: if we do get a convincing reason that this is really faulty we can always pivot, after all it's only an early alpha build with a long way to go.

@JamesBremner
Copy link
Author

The label_specs class in this file and the label_impl class here.

What is this! Another label class

A label is just about the simplest widget imaginable. I dislike the fact that you already need to know about two classes to implement a label using your library ( label_specs and label_builder ) Now you have a third class!

Each widget should have exactly one class. Anything more renders you library too complicated to use.

@JamesBremner
Copy link
Author

You must be kinder to your users.

If someone is going to go to all the trouble to learn how to use your library, they need to expect a benefit for their work. Either, interesting new features, or simplification of their application code.

A gui framework offers no new features ( everything is already present in the Windows API ) so you have to offer simplifications. Your complex class design ( three different classes to implement a label widget! ) makes it harder to use than the windows API, not simpler.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

I was really hoping you wouldn't end up speaking like this because now it really sounds like you're just talkind down the library without really hearing what I'm saying. The user doesn't need to know anything except the label_specs class. The builder was a recent thing if you look at the repo history. A few months ago to be precise. Look at the repo history. label_impl is the internal implementation and the caller doesn't even know about it.

There should be only one label class

I disagree. The implementation class needs to be different. Look at boost, poco, etc they all use some form of pimpl idiom class for implementation.

But then again with the tone you're now using I don't see the point of this conversation anymore.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

And just so you know, the design was necessary because of the multi-page framework and also the way widgets and containers can be dynamically created and destroyed at runtime. Look at pc_info which was built using this library and see how it can dynamically change it's interface easily when devices are plugged and unplugged etc. And how it can change widget dimensions and so on when updates are found. That kind of behaviour built using such little code is only possible because of the architecture of this library which you seem determined to discredit.

Download it and test it on your pc perhaps you'll see things differently.

@JamesBremner
Copy link
Author

the multi-page framework

What is multi-page framework? Do you mean multiple top level windows?

the way widgets and containers can be dynamically created and destroyed at runtime.

Widgets can do this in the windows API, the windex library and every other framework I have ever used. There is no need for the complexity you have added.

@JamesBremner
Copy link
Author

with the tone you're now using I don't see the point of this conversation anymore.

A suggestion: Ignore the tone, focus on the content. I am trying to point out the mistakes you are making that I have learned about by making them myself. If you will only accept advice from someone willing to spend the time to stroke your ego, then I am not the person you need. Good luck finding someone both knowledgeable AND diplomatic.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

To be objective, can you honestly say that this below is more complex than winapi in making a formatted label that can respond to a mouse click, the enter key and the spacebar?

Only this:

auto& home = _page_man.add("home");

// make a label
auto& label = lecui::widgets::label_builder(home)();
label
   .text("This is a simple <strong>label</strong>.")
   .events().action = [this](){ message("Label clicked!"); };

To get this:

screenshot

More complex than the Windows API? The things one has to do to get the same result, and imagine what you have to do to make one word bold like that.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

What is multi-page framework? Do you mean multiple top level windows?

No. I mean pages within the same form without opening any modal dialogs or any other forms. I mean like how javafx uses scenes and stages. The lecui form is analogous to a javafx stage and the lecui page is analogous to a javafx scene. You can change the scene by simply calling:

_page_man.show("sample_page");

This changes the entire contents of the form to something else allowing you to build an application that can browse from page to page, that is why we called it a page to begin with. Doing the same thing in WinAPI is a complete nightmare believe me I used to do it.

An example of where this is used is in the Next button in this sample app below:

screenshot

And the "Back" button in the next page:

screenshot
(note that the form was resized after advancing to the next page to demonstrate how fluid the resizing of widgets is using hardware accelerated graphics when available.
Clicking the "Back" button takes you back to the home page, all within the same form, much like a web browser. And all that is done with this code:

auto& btn_back = lecui::widgets::button_builder(second_page, "btn_back");
btn_back()
    .text("Back")
    .events().action = [&]() { _page_man.show("home_page"); };

That's what I mean by multi-page.

@alecmus
Copy link
Owner

alecmus commented Jul 30, 2021

Ignore the tone, focus on the content.

Granted, and I have been trying my best. Just that the argument you presented isn't holding regarding the getters and setters within this particular context, and furthermore you didn't seem to be making an effort to really hear what I was saying regarding the unique architecture I used. What's the point in all the libraries being the same? I find it very useful and managed to build the first working version of pc_info within a space of 4 hours using this library and leccore. It's that easy once you understand it. Every framework requires a learning curve but once one grasps it everything changes. Now I can build applications very rapidly using this library and it has never been easier for me, honestly. So much that I decided to share it. Also, sharing it gives the opportunity for input, which I gladly listen to ... but I am also allowed to disagree when something is said that is contrary to what I believe and understand am I not?

@alecmus alecmus closed this as completed Jul 30, 2021
Repository owner locked and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants