245 replies to this topic

[littlesat]

Please discuss your ideas first before implemented some code and get disappointed afterwards...I know you are used to quickly create some code.

 

Note that indeed everything is solvable.. However before you know they want to have icons configurable on first line, or second line, left right, upside down, downside up, diagonal etc. etc... so you can end up in a never ending story. Sometimes you should say "STOP'...

 

And maybe put it on hold and let this skin being completed....

 

This is all solvable.

 

Edited by littlesat, 15 December 2023 - 10:39.

[Huevos]

This is all solvable.

This is true.

 

You have a choice,

1. you can have a lot of spag-bol in the python code
2. keep it simple and have an appropriate description in the settings

Edited by Huevos, 15 December 2023 - 10:43.

[littlesat]

I hope I can see a merge request soon.... so we have everything in the code for this skin and we can finetune it...

Edited by littlesat, 15 December 2023 - 11:18.

[DimitarCC]

here is the pull

 

https://github.com/O...igma2/pull/3872

 

Have to be checked ofcource how will behave with different settings on PLi. I didnt ran complete tests.

[DimitarCC]

OK but PLi compiler threats one thing as an error instead of warning. But it is required to be in this way so to have localizable string in there.

service/listboxservice.cpp:1168:107: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
 1168 |       snprintf(buffer, sizeof(buffer), ("%s" + m_text_time).c_str(), timeLeft == 0 ? "" : "+", timeLeft/60);

I have no solution for that just because the compiler threat it as error instead of warning....

 

It works just fine on ViX....

Edited by DimitarCC, 15 December 2023 - 11:50.

[littlesat]

service/listboxservice.cpp: In member function ‘virtual void eListboxServiceContent::paint(gPainter&, eWindowStyle&, const ePoint&, int)’:
service/listboxservice.cpp:1168:107: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
 1168 |       snprintf(buffer, sizeof(buffer), ("%s" + m_text_time).c_str(), timeLeft == 0 ? "" : "+", timeLeft/60);
      |                               

I think this is a strange construction.... I think it is obvious that VIX does not give errors....

 

snprintf(buffer, sizeof(buffer), ("%s" + m_text_time).c_str(), timeLeft == 0 ? "" : "+", timeLeft/60);

 

The have one %s and you try to add a '+' or space and a text timeLeft/60....  there are to things added and we only have a string to add...?

 

Maybe something like this?

 

snprintf(buffer, sizeof(buffer), (timeLeft == 0 ? "%d " + m_text_time : "+%d " + m_text_time).c_str(), timeLeft/60);

 

 

 

Even when it is a warning it should be improved...

Edited by littlesat, 15 December 2023 - 12:32.

[littlesat]

I also see at least one whitespace... line 870.

[DimitarCC]

service/listboxservice.cpp: In member function ‘virtual void eListboxServiceContent::paint(gPainter&, eWindowStyle&, const ePoint&, int)’:
service/listboxservice.cpp:1168:107: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
 1168 |       snprintf(buffer, sizeof(buffer), ("%s" + m_text_time).c_str(), timeLeft == 0 ? "" : "+", timeLeft/60);
      |                               

I think this is a strange construction.... I think it is obvious that VIX does not give errors....

 

snprintf(buffer, sizeof(buffer), ("%s" + m_text_time).c_str(), timeLeft == 0 ? "" : "+", timeLeft/60);

 

The have one %s and you try to add a '+' or space and a text timeLeft/60....  there are to things added and we only have a string to add...?

 

Maybe something like this?

 

snprintf(buffer, sizeof(buffer), (timeLeft == 0 ? "%d " + m_text_time : "+%d " + m_text_time).c_str(), timeLeft/60);

 

 

 

Even when it is a warning it should be improved...

 

 

No the problem is not that. The problem is that %d comes from localized text in m_text_time. And since that is variable and not literal the compiler doesnt find the %d in format literal but there is argument of type integer...

 

Sory but that is stupid since now when i pass localized text have to strip the %d from it....What for?!

[littlesat]

he problem is that %d comes from localized text in m_text_time -> Simply do not do that as simply the compiler cannot now the content... ERROR is what it should give here... Sounds like VIX (or oe-a) played with the compiler so such things are allowed... This is also not 'structured' programming as in c++ you cannot see what is really happening.... I know this is a pain.... Better keep the %d visible in cpp.. makes readable code and only translate the text... b.t.w. this field is even new...

 

At least we understand how to fix this and I know in python you do not see the %d then for the translation... When we want to have it still there then indeed we need to strip it off but we can than (later) if it is kept decide were it may appear in the text...

 

Unless this I think it is valid this is an ERROR and not a warning... because in the string the %d is not guaranteed there... and the compiler does really not know what is inside that string... What should it do if the %d is really not there? I think it is a 'wonder' that VIX does compile it... or give just a warning. 

 

I checked the rest of the code when this is resolved then as far I can see it can be merged...

Edited by littlesat, 15 December 2023 - 12:54.

[littlesat]

P.S.

 

I just saw this in the selection options...

(only available in single line mode)

 

I meant put this remark in the description. So in the setup.xml... and not in the code... this makes the options so long....

 

Again me... I saw it is also there... doing it in the description only should be sufficient and smoother.

Edited by littlesat, 15 December 2023 - 12:58.

[Huevos]

Sounds like VIX (or oe-a) played with the compiler so such things are allowed...

This is not true. In 2020, as an OpenPLi Christmas holiday project, "eriksl" thought it would be a good idea to convert a lot of compiler warnings to errors.

 

https://github.com/O...op/configure.ac

 

https://github.com/O...22036312670e4dc
https://github.com/O...faa78d7f0c05b7a
https://github.com/O...8b8aa7ae96e6758
https://github.com/O...1eeb00bdc7cc58a
https://github.com/O...80ba5d1547e67a0
https://github.com/O...24eb109a9ef951a
https://github.com/O...34d9280e0b354cd
https://github.com/O...846abf19f7062b1
https://github.com/O...38a591fe7d0961e
https://github.com/O...bb56bc12b549bd2

Edited by Huevos, 15 December 2023 - 13:57.

[Dimitrij]

Maybe?

timeText = _("min")

snprintf(buffer, sizeof(buffer), "%s%d %s", timeLeft == 0 ? "" : "+", timeLeft/60, m_text_time.c_str());

 

[DimitarCC]

 

Maybe?

timeText = _("min")

snprintf(buffer, sizeof(buffer), "%s%d %s", timeLeft == 0 ? "" : "+", timeLeft/60, m_text_time.c_str());

 

There is no such a  localization right now. And this leads to unpredicted results.

[littlesat]

Indeed... but eriks made the right decision here as far I know... ignoring these as warnings could lead to unpredicted stuff... what if the %d is not there?....  But you're right... we changed the stuff that it becomes an error instead of warning.

 

Please adjust it... (even warnings should be minimized!!!)

 

This is not true. In 2020, as an OpenPLi Christmas holiday project, "eriksl" thought it would be a good idea to convert a lot of compiler warnings to errors.

 

Edited by littlesat, 15 December 2023 - 14:12.

[DimitarCC]

he problem is that %d comes from localized text in m_text_time -> Simply do not do that as simply the compiler cannot now the content... ERROR is what it should give here... Sounds like VIX (or oe-a) played with the compiler so such things are allowed... This is also not 'structured' programming as in c++ you cannot see what is really happening.... I know this is a pain.... Better keep the %d visible in cpp.. makes readable code and only translate the text... b.t.w. this field is even new...

 

At least we understand how to fix this and I know in python you do not see the %d then for the translation... When we want to have it still there then indeed we need to strip it off but we can than (later) if it is kept decide were it may appear in the text...

 

Unless this I think it is valid this is an ERROR and not a warning... because in the string the %d is not guaranteed there... and the compiler does really not know what is inside that string... What should it do if the %d is really not there? I think it is a 'wonder' that VIX does compile it... or give just a warning. 

 

I checked the rest of the code when this is resolved then as far I can see it can be merged...

 

Default c++ behavior in c++11 is that to be a warning not error. You (PLi) has played with the compiler to make all warnings errors. This is not the default behavior for sure.

 

If you make that error in that particular case you limit the functionality of c++ without obvious reason imho. You can not construct dynamic format strings just because of that. That is not right.

 

Anyway i fixed the code for PLi, But that not means that is right....

[littlesat]

Yes you're right... but also warnings should be avoided... you build in something that can be unpredicted and could behave randomly and even could lead to crashes... e.g. when something is done and the %d is not there...

It was not for nothing erik did change it... maybe during that corona holiday period he also removed a lot of warnings... at least he forced himself to do it....

There is no limitation in C++ at all... it is simply wrong...

[DimitarCC]

 

 ignoring these as warnings could lead to unpredicted stuff... what if the %d is not there?....

 

 

As i stated above for that particular case that is not right since you limit functionality. How you can construct dynamic format strings? You simply can not. Then why they put that possibility in c++. If it was that bad they should have prevent it by default.

Ofcource it can lead to errors if %d is not there. But that is developer mistake where he didnt put the correct variable.

Edited by DimitarCC, 15 December 2023 - 14:16.

[littlesat]

Yes indeed it limit the functionality but is is still wrong what you try to do here.... the compiler does simply not know what is in the string....

I know on oe-a images they accept a lot... but this is an example from how to introduce spaghetti in the the code....

This is one line of code that needs attention.... Note the translation will also work without the %d swigged from python... and when it is in we need to ensure the compiler knows it is in. So we do not get a Warning in Vix.... And we do not get an Error in openPLi. When the %d it is not defined how it may behave!!! That is where the warning/error is for!

Edited by littlesat, 15 December 2023 - 14:21.

[DimitarCC]

There is no a single language in which that is error and it is prevented. There must be a reason for that.

 

Anyway further discussion about that is pointless. I have made it different only for PLi...So you can merge it.

[rantanplan]

 

he problem is that %d comes from localized text in m_text_time -> Simply do not do that as simply the compiler cannot now the content... ERROR is what it should give here... Sounds like VIX (or oe-a) played with the compiler so such things are allowed... This is also not 'structured' programming as in c++ you cannot see what is really happening.... I know this is a pain.... Better keep the %d visible in cpp.. makes readable code and only translate the text... b.t.w. this field is even new...

 

At least we understand how to fix this and I know in python you do not see the %d then for the translation... When we want to have it still there then indeed we need to strip it off but we can than (later) if it is kept decide were it may appear in the text...

 

Unless this I think it is valid this is an ERROR and not a warning... because in the string the %d is not guaranteed there... and the compiler does really not know what is inside that string... What should it do if the %d is really not there? I think it is a 'wonder' that VIX does compile it... or give just a warning. 

 

I checked the rest of the code when this is resolved then as far I can see it can be merged...

 

Default c++ behavior in c++11 is that to be a warning not error. You (PLi) has played with the compiler to make all warnings errors. This is not the default behavior for sure.

 

If you make that error in that particular case you limit the functionality of c++ without obvious reason imho. You can not construct dynamic format strings just because of that. That is not right.

 

Anyway i fixed the code for PLi, But that not means that is right....

 

No, you should look at it the other way around.

We compile with a more recent version and would fool the system using the c11 flag.
This is done to keep something running, because it's too convenient to fix the errors that are in c17.
I really don't know much about programming languages, but something like this should be obvious to everyone.

Either you compile with the old compiler version c11 (i.e. gcc4) or you also repair errors.