News:

Do you need help?
Simutrans Wiki Manual can help you to play and extend Simutrans. In 9 languages.

[Project] GUI Theme

Started by Max-Max, May 31, 2013, 11:12:48 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Max-Max

Quote from: prissi on October 12, 2013, 07:53:28 PM
Microsoft really does not won't me working this weekend. While editing file Windows7 did a restart to install patches. Without a warning, not asking for saving, just restart while I was typing. (XP did this about once a year, but I never expected it from Win7). I lost the last two hours. Sorry, will not finish today then.
Really? That never happened to me, are you sure you have turned off automatic updates?
- My code doesn't have bugs. It develops random features...

Markohs

You really need to save files more often.  :-)

Markohs

You really need to save files more often.  :-)

IgorEliezer

Quote from: Markohs on October 12, 2013, 11:01:14 PM
You really need to save files more often.  :-)
You really need to post the same thing so often? ;)


Markohs

My phone lost wi-fi and accidentally double posted, and I don't know how to delete posts. ;-)

isidoro

Quote from: Markohs on October 12, 2013, 11:01:14 PM
You really need to save files more often.  :-)

I'd rather say: "You really need to change your Operating System."   ;)

Markohs

Well, linux doesn't force auto-save of edited files neither, lots of things can happen to a computer, not saving open files for one hour you are calling this happening to you. :)

Anyhow I know Windows 7 does that, you just need to change system updates to require conformation. I agree it should never reboot without asking for conformation.  Linux will do that the sooner or the later, seeing they are just going that direction lately, specially Ubuntu. :)

Max-Max

Hmmm, maybe we should try to stick to the topic here, no need to rub it... I guess we all have been there...  :police:
- My code doesn't have bugs. It develops random features...

prissi

Indeed, let's not get offtopic. I worked through the patch, and I am closer to submit it. Still not finished though.

First some observations
- Convoi dialoges do not display nicely with large buttons
- Factory dialogues do not open with standard size any more (but that may be even better)
- First start will give no visible buttons at all, just plain text. I think a theme shoudl be enforced and missing images not tolerated.
- You assumed a system with lowercase==uppercase. (Incidentally your directory is "Themes".) That will not work anywhere but on windows. But removing to_lower is trivial though.

Some things that probably need more though:
- I am not sure about location and function of "global.tab". If such a thing is required, it should go into the user dir. And then one might think of using simuconf.tab for this. However, I fail to see the advantage of global theme settings, resp. many setting will not work with certain themes (like certain colors).
- The themeselector does apply themes without closing itself. That is not how the other selector work (although I introduced it this way).
- The drawing of color buttons should rather use a mask image which is then blended before the final button is drawn. The rectangle will not work on certain buttons designs (like asymmetric with a colored tab on the left.) So it would need up to 10 images for a fully scaleable button.
- button_t should be a pure virtual function and all buttons should be derived from it. This will automatically force the correct init functions (liek with string or without, with sizes or without, with a position or without). The impact on the construction of dialogues will be rather small; instead definition of one array of buttons more than one with different elements are needed. At that time some of the long action callback functions could be split into more, depending on how much actions they join.
- All theme image under skinverwaltung_t::theme is not needed and does not simplify anything.
- Finally the chopping of images by the Simutrans exe. As stated times before, it always turned out better to give the artist more freedom. Sorry, since you put much effort in decoding and cutting. But I am very deeply convinced that this must belong to the artist, not the program. Also certain asymmetric button designs (or the masked ones) seem difficult to be cut up like this but I have to think on that more. The patch I submit will most likely miss that part (for now). I really want to hear more artist input on that.

Max-Max

Quote from: prissi on October 13, 2013, 10:56:30 PM
First some observations
- Convoi dialoges do not display nicely with large buttons
- Factory dialogues do not open with standard size any more (but that may be even better)
Well observed. I wrote that I haven't done anything about the dialogues. This will be the next step...

Quote from: prissi on October 13, 2013, 10:56:30 PM
- First start will give no visible buttons at all, just plain text. I think a theme shoudl be enforced and missing images not tolerated.
I have not yet implemented any fallback at this point. There is an empty validation routine that will take care of this.
For some one who are developing Simutrans, fall backs might seem unnecessarily. If Simutrans won't start we usually know why.
A first time user starting Simutrans, only to find it close immediate, or leave a cryptic error message might think "what a crap program" and never try it again...

Quote from: prissi on October 13, 2013, 10:56:30 PM
- You assumed a system with lowercase==uppercase. (Incidentally your directory is "Themes".) That will not work anywhere but on windows. But removing to_lower is trivial though.
I'm fine with all lower case, I'm only on Window based systems so I'm not used to think about these file name constraints...

Quote from: prissi on October 13, 2013, 10:56:30 PM
Some things that probably need more though:
- I am not sure about location and function of "global.tab". If such a thing is required, it should go into the user dir. And then one might think of using simuconf.tab for this. However, I fail to see the advantage of global theme settings, resp. many setting will not work with certain themes (like certain colors).
The global theme setting is to ensure that you get a certain size of your GUI no matter what. This is important on mobile devices where the GUI needs to be big enough to use. Sure not all themes would look "nice" but at least be operational. I'm sure the user will select a theme he likes and fits his purposes.
It is not that the global.tab by default makes all GUI elements huge in size. It is something intended for users who need it on mobile devices without having to modify every theme before he can select one.

Quote from: prissi on October 13, 2013, 10:56:30 PM
- The themeselector does apply themes without closing itself. That is not how the other selector work (although I introduced it this way).
It should absolutely not close itself for three main reasons.
  1) To easily be able to browse and preview the themes, the dialogue can't be closed in between, it would make it very un-user friendly to select a theme.
  2) If the selected theme for some reason screw things up, you must be able to select another one.
  3) As a Theme designer you can do a change and then easily test it by just press the same theme again in the theme selector to reload it. Other wise he has to go through a series of dialouges before he can reload his modified theme.
You might also have seen that there is a Cancel button if you aren't happy with your choice, it reloads whatever you had before you opened the theme selector.

Quote from: prissi on October 13, 2013, 10:56:30 PM
- The drawing of color buttons should rather use a mask image which is then blended before the final button is drawn. The rectangle will not work on certain buttons designs (like asymmetric with a colored tab on the left.) So it would need up to 10 images for a fully scaleable button.
I know all about it. But it is better than nothing right?
I do have an idea of how to automatically create a mask, use a special colour or simply provide a mask... it is in the pipe...

Quote from: prissi on October 13, 2013, 10:56:30 PM
- button_t should be a pure virtual function and all buttons should be derived from it. This will automatically force the correct init functions (liek with string or without, with sizes or without, with a position or without). The impact on the construction of dialogues will be rather small; instead definition of one array of buttons more than one with different elements are needed. At that time some of the long action callback functions could be split into more, depending on how much actions they join.
- All theme image under skinverwaltung_t::theme is not needed and does not simplify anything.
- Finally the chopping of images by the Simutrans exe. As stated times before, it always turned out better to give the artist more freedom. Sorry, since you put much effort in decoding and cutting. But I am very deeply convinced that this must belong to the artist, not the program. Also certain asymmetric button designs (or the masked ones) seem difficult to be cut up like this but I have to think on that more. The patch I submit will most likely miss that part (for now). I really want to hear more artist input on that.
Prissi, have you read any of my posts at all? Have you read my document? Have you even read the first post in this tread?
I have a vision, a plan and a goal. But I can't implement everything at once because you don't want to big patches. This means you will find things not being fully developed or implemented. I can't just remove stuff without having something replacing it. I also try to submit patches that make as little impact as possible on current system. As long we use the "old" skin format there are some limitations, but it's still an improvement compared to the old skin files.

I will say this again, and hopefully for the last time:

- skinverwaltung_t::theme will be removed, it is not needed as soon the theme manager can load the theme images by itself. It is still used for the convenience of using an already working system instead of a new untested one.

- The gui classes will be redesigned in a more strict hierarchy to reuse code and behaviours. I have not done this yet because at first it will not contribute anything to the end user and meanwhile this reconstructing is done, the code will be quite useless (unless we run parallel systems meanwhile). I thought it was better to first give the end user some more candy before I star to work on this.

- The artist will be able to shop up the images, set text offset, colours, masks etc... but we need a new pak format for this. Meaning new readers and writers, a scanner, tokenizer and a parser.

Do not remove any code, you will take away a lot of the infrastructure I have built to use as a base for coming features. I can't in my wildest imagination think that an artist would oppose the possibility to draw a button in one or two or three cells depending on how he wants it to be cut. If you think that all GUI elements are cut in equal pieces, then you have not understood the code.


All this will be done in the end, but it goes really slow because I can't check in stuff myself. I'm depending on those who can and when they have time for it. If we on top of this also debates about small details, imaginary problems or remove code we "don't like", I also get lesser time to do coding and move the project forward.

I think what I have submitted now, even with a few limitations, are far better than the skin system we had before. The end user only sees what's on screen, not how it is implemented (and probably doesn't care either). The work will continue and next up is to fix the remaining dialogues if there aren't any more urgent things to fix before that. But if you are to remove code because you don't "like it", this project is doomed and I see no point in continuing...

If some one disagree or agree with me please step forward...
- My code doesn't have bugs. It develops random features...

kierongreen

QuoteIf some one disagree or agree with me please step forward...
Just so you know people are reading what you say - I can't agree or disagree on this. From my own experience I know that developers are reluctant to include code in trunk until there is an actual need for it. Until then it may just seem an unnecessary complication.

Markohs

#432
I prefer allways to go with the simplest design that works, I don't like seeing code that's not used, just because it was pre-designed that way in the hopes of using it later.

I'd say patches should *never* include code that's not being used, because if you are not using it, you will probably not use it in the future, and when you are going to use it, you prolly designed it in the wrong way.

The best way of designing is just implement in the go, just implementing what you are going to use, *now*.

http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it
http://en.wikipedia.org/wiki/KISS_principle

Including provisioned code just for the future will take to the place you are right now, because now developers are arguing about a design that's not even used. You are on a weak position , trying to defend something without many arguments, just defending "air". And that's not a good position to be. ;)

My advise is the same I gave you before, program more, implement more, when results are evident, you'll be able to defend your design way better. Even I understand you, Max, and my wild guess is your design makes sense. But you can't prove it. ;)

Hope to see more of this new code soon! :)

prissi

I put a lot of effort in the GUI system. Even though, I certainly see that there is lot of room for improvement.

Let us go back to your first post:
Quote
Road map

Convert all GUI elements to use the elements defined in frame_t.h instead of all the magic numbers. This may result in additional element definitions.
1) Set each element size to the size of its skin image.
2) Create a true window class, gadget bar class and a gadget class
3) Introduce a true client relation and update all dialogs
4) Modify/add controls if needed to work better with the concept of resizing
5) Create a dialog to select and apply a skin (theme).
6) Add detection and special handling for a small screen area (example portable devices).

1) and 5) is done, 4) is somewhat tackled by this theme manger, which I do not like. But on the basis of this list, the thememanager is not even menitioned.

I think we can agree on many things concerning the GUI: That the current state of buttons is not good, many switch statements were not very OOP. That the dialogues were hardcoded and that elements need to align much more relative to each other. That there are configurable themes and more flexible designs. Even that the window should be maintained by a class and not from the code of a C-file (like simwin.c was) And the artist may not care about how the button is draw, he cares about how much control he would get over appearance. Hew would certainly not mind the freedom of having scalability or not, you are right.

But as long as the dat format is documented, an artist does not call whether he has to write "Image[1]" or "Image[button-pressed]" or whatever, which we probably disagree. But unfourtunately we disagree on a fundamental level. The core of disagreement is here:
Quotebut we need a new pak format for this. Meaning new readers and writers, a scanner, tokenizer and a parser.
It is nice to change everything for the sake of change and challenge. But *why*? Simutrans has a working pak system, everything in the Simutrans universe in the last 10 years or so is built around it. There is *absolutely no need* to break this. It can be achieved quite easily in current format, or you can extend it (like button definitions in 2D image list, with "phase 0" normal, "phase 1" pressed and "phase 2" disabled.

On there very core of this topic: The themeability of Simutrans has made no progress by this patch (dialogues are still visually not compatible with the new sizes). It may be an unfair statement, I agree.

So this is how is stands: I do not like another Hajo sized flamewar on Simutrans. Please accept to reuse the existing infrastructure for what it is worth (you will see this when the patch is submitted). Or we have to agree that I cannot make my ideas compatible with yours on a fundamental level and we have to depart (which I would regret).

Max-Max

Quote from: prissi on October 14, 2013, 08:56:14 PM
But as long as the dat format is documented, an artist does not call whether he has to write "Image[1]" or "Image[button-pressed]" or whatever, which we probably disagree. But unfourtunately we disagree on a fundamental level. The core of disagreement is here:It is nice to change everything for the sake of change and challenge. But *why*? Simutrans has a working pak system, everything in the Simutrans universe in the last 10 years or so is built around it. There is *absolutely no need* to break this. It can be achieved quite easily in current format, or you can extend it (like button definitions in 2D image list, with "phase 0" normal, "phase 1" pressed and "phase 2" disabled.
Well, I have not explained exactly how it would work so you are only speculating and assuming that I will "invent" a complete new pak file format. I'm no expert on writers/readers but what I have seen so far, It looks like each writer class is doing its own scanning and parsing, even its on white space filtering. In the end, this design allows each individual line in a .dat file to have a complete different syntax from another.

If we used a scanner and a tokenizer ALL .dat and .tab files would have the same syntax, you would get something that people recognize and can use in all .dat and .tab file creations. The parser might be specialised for different purposes, but could be shared among the most writers. This would not only give us a homogeneous configuration language, but also syntax checking already at "compilation", not a crash when testing.
The .dat variables that now are hard coded to a child node's index, makes it inflexible. By adding a text node with all variables coded with name and its binary value would make it easy to search for a specific variable, not worry about what index it is located at. This would move the reading up to a more high level so a standard reader could be used and simply just pick the information that is known.

The only new in the PAK file would be the new text node with all named variables. Backward compatibility. The reader can look for the current index based variables and then in the new text node.
To include all the things I want in the theme PAK file, such as element size, behaviours, colours, text offsets etc... The use of a text node is the most flexible because we can then add or remove variables without changing the low level writer or reader.

Further more I have talked about a guard colour that the artist will use to show how the GUI control images should be cut. By using this and have the theme manager to detect these lines we are not only backward compatible with existing skin format but also making use of the current .pak format. See the current images as a transport layer for an extended format, detected and used by the theme manager. What is it that is so bad about this design? The artist get a lot more flexibility and we still use the same old pak format and as a bonus we get a homogeneous syntax for .dat and .tab files. What is it in this you don't like?

If you look closely at how I use the current skin-pak format you will see that it works just as before with even more flexibility.

Quote from: prissi on October 14, 2013, 08:56:14 PMSo this is how is stands: I do not like another Hajo sized flamewar on Simutrans. Please accept to reuse the existing infrastructure for what it is worth (you will see this when the patch is submitted). Or we have to agree that I cannot make my ideas compatible with yours on a fundamental level and we have to depart (which I would regret).
Maybe this is one of the problems, that parts who really needs some design changes isn't "allowed" to change... ???
I have no idea of who Hajo was or is, what kind of quarrels you two had, but maybe he had a great vision and got frustrated because some one kept deleting his code with the argument "this isn't needed" ???

Quote from: prissi on October 14, 2013, 08:56:14 PMOn there very core of this topic: The themeability of Simutrans has made no progress by this patch (dialogues are still visually not compatible with the new sizes). It may be an unfair statement, I agree.
Really... So we have no filter button?, we have no sizeable buttons? we have no disable states? No auto hidden scrollbars? No configurable colours? I have just wasted 400 hours on nothing? Is this what you think of my work?
I was really excited about this patch because I thought I was introducing more eye candy for the user and more flexibility to the artists. The screen shots I posted got really good comments from artists, but I guess we all where wrong... I'm sorry I put so much work into something so "useless".

Quote from: Markohs on October 14, 2013, 01:01:42 PMI'd say patches should *never* include code that's not being used, because if you are not using it, you will probably not use it in the future, and when you are going to use it, you prolly designed it in the wrong way.

Including provisioned code just for the future will take to the place you are right now, because now developers are arguing about a design that's not even used. You are on a weak position , trying to defend something without many arguments, just defending "air". And that's not a good position to be. ;)
The code in question is in use... Code I had commented out in scr_coord.h because it was not used has been enable again, so I really don't get the logic here; enable code not in use and remove code in use...  :o

Quote from: Markohs on October 14, 2013, 01:01:42 PMMy advise is the same I gave you before, program more, implement more, when results are evident, you'll be able to defend your design way better. Even I understand you, Max, and my wild guess is your design makes sense. But you can't prove it. ;)
Sure I can do my "thing" and then upload a huuuge patch and you guys will have quite some job to implement it, that isn't really my problem. But I agree that it is best to update small sections at the time. It will make it easier for you and me, but all updates will not necessarily contribute with something magnificent. It just makes it easier for us to keep in synch.

Quote from: Markohs on October 14, 2013, 01:01:42 PMHope to see more of this new code soon! :)
You would if someone didn't changed my code all the time. I'm not sure I want to do this any more...

If you try to build a house, you have to start with the ground, then the first floor and so on. How can you at the end put up the roof if someone decides that no walls are needed and removes them, because he can't see any roof yet  ???
If the stairway to the second floor isn't finished yet, but there is a temporary ladder in place. Is this an argument to remove the whole second floor?
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on October 14, 2013, 11:04:47 PM
If we used a scanner and a tokenizer ALL .dat and .tab files would have the same syntax, you would get something that people recognize and can use in all .dat and .tab file creations. The parser might be specialised for different purposes, but could be shared among the most writers. This would not only give us a homogeneous configuration language, but also syntax checking already at "compilation", not a crash when testing.

dat and tab files already share their syntax.

Quote from: Max-Max on October 14, 2013, 11:04:47 PM
The .dat variables that now are hard coded to a child node's index, makes it inflexible. By adding a text node with all variables coded with name and its binary value would make it easy to search for a specific variable, not worry about what index it is located at. This would move the reading up to a more high level so a standard reader could be used and simply just pick the information that is known.

I just looked through the dat-files for pak64 and couldn't find many places where indicies are used without it being natural. As far as I can see, the only places with cryptic indicies are building rotations, signals and whatever one should call the indicies for grounds. To a lesser extent also seasons. For grounds, I seem to remember that the indicies for grounds makes perfect sense inside the code. Coming up with meaningful names might be impossible, or the names might be so long that nobody wants to have to write them. Buildings are cryptic to me either way. Seasons work fine as indicies in the pak, but could use symbolic names like normal rotations have.

Signals are the only type I could find where indicies map directly to unstructured pak file indicies. These really should be written with separate direction and state in the dat file. (There is also the electrification issue, but that is a different issue completely.)

Where the indexed nature of pak files really is a bad thing is not when writing dat files, but when the game reads a pak node where a variable number of child nodes have been flattened out into single list. Looking up a particular node then involves a rather complicated index calculation that has to take into account how many preceeding child nodes are present.

Quote from: Max-Max on October 14, 2013, 11:04:47 PM
If the stairway to the second floor isn't finished yet, but there is a temporary ladder in place. Is this an argument to remove the whole second floor?

I think the argument here is that there isn't even a temporary ladder. There is no way to get to the second floor at all, not even a hole between the floors yet. (Although I haven't myself checked if that's the case.)

Dwachs

I think we are turning cycles in this discussion.

Max-Max, it would be really good if you could split your large patch into smaller pieces. Explain how these pieces fit together and depend on each other. You have put a lot of effort into this patch, which should not be wasted.

Prissi, I hate to say this, please post your modifications before committing modified parts of the patch. If you do not like to work on this patch, then please say so. I can do this as well.

About the pak/dat-stuff: There is already a parser at hand: tabfile_t does the low-level parsing. It creates an array/table with string keys and values. There is room to remove code duplication, I agree, as each writer-class puts generates the keys using string buffers. Changing the skin-writer class to use string keys instead of integers is favorable ofc, but the necessary change can be done on high-level (the writer) without changing low-level stuff (parser / tabfile).
Parsley, sage, rosemary, and maggikraut.

Markohs

Quote from: Max-Max on October 14, 2013, 11:04:47 PM
The code in question is in use... Code I had commented out in scr_coord.h because it was not used has been enable again, so I really don't get the logic here; enable code not in use and remove code in use...  :o

My excuses then, I was just commenting based on previous comments in this posts, I didn't actually look into the code.

Quote from: Max-Max on October 14, 2013, 11:04:47 PM
Sure I can do my "thing" and then upload a huuuge patch and you guys will have quite some job to implement it, that isn't really my problem. But I agree that it is best to update small sections at the time. It will make it easier for you and me, but all updates will not necessarily contribute with something magnificent. It just makes it easier for us to keep in synch.
You would if someone didn't changed my code all the time. I'm not sure I want to do this any more...

If you try to build a house, you have to start with the ground, then the first floor and so on. How can you at the end put up the roof if someone decides that no walls are needed and removes them, because he can't see any roof yet  ???
If the stairway to the second floor isn't finished yet, but there is a temporary ladder in place. Is this an argument to remove the whole second floor?

I was just speaking in general, and not of any of the details of your implementation, that I don't know enough. If it was for me, I'd just give you green light to incorporate all your code. After all this is a open sourced project, and we use a versioning system so we can allways rollback any changes if they come undesired with time. And we have to accept everyone has different programming styles. If your approach to the isue is proved to be wrong, or partially wrong, we can allways refactor it, or throw it to the trash 100% and start again from scratch.

What I was trying to point you is my vision of how design should be in my oppinion. And I strongly believe you should not even think about the roof, before you finished the first floor at 100%, experience taught me so. Once you start second floor, you'll prolly need to modify first floor, but just sightly, and if it's a heavy modification, you'll know exactly how you need to modify it, because you have extra information. But of course it's just my personal opinion. :)

About pak file format, I hate it too, but it's what we have, and it has proven to work quite good. I'd just suggest you to use it, resign to it, and once you are to the point you can't step forward because of the limitations of that format, you'll have a strong point to suggest a change. If you are positive about pak, and start using it, you'll find many good parts of the old system, and maybe you change your oppinion. It's better to know your enemy well. ;)

When I read about cut colors on the gui element it sounded like a good idea to me, I don't know how did that end.

I won't comment more on this issue, to not grow the issue more and speak more about this in circles.

prissi

I am nearly done, but have to fix some small errors first. I will publish the patch (based on Max-Max one) hopefully tomorrow evening (currently all children are with fever, so short nights for me). Sorry for the delay.

kierongreen

I hope your children get better soon prissi!

prissi

They did not, rather now my throat is sore ...

I did not finish the designs yet, so also I would consider this as a work in progress. I did not finish the standard theme yet, and also not the Windows7 button discussed earlier (even though the infrastructure is mostly in place).

ThemeWorked9.zip contains the patch, themes.zip the new theme paks for the simutrans folder, and aero-src.zip the sources of the theme pak.

Max-Max

#441
As you say, it's work in progress so I will not judge you to hard... yet.
By looking at the theme source I noticed you have reinstated "your" version of the theme .dat file and graphic format, I assume you must have thrown out quite a lot of my code and constrained the artist to the old one-and-only way to draw graphics, not my simpler addition to the current format.

I just don't understand why you can't just apply my full patch and have the community to have a look and say what they think before you start to throw away my work. Are you always censoring everyone's code or is it only mine :o (feels like that), I thought this was open source and any one could contribute.

I will stop to write now because I'm quite pissed and will probably say something I will regret later
- My code doesn't have bugs. It develops random features...

Dwachs

#442
I feel we are heading full speed into a wall here.

Max-Max, it would REALLY help your case, if you could split your gigantic patch into smaller, easier to review parts. From looking at the patch, you have several different lines of development there: debug output for makeobj, named constants in simskin, alot of theme-stuff-* etc. The patch says nothing about the purpose of these theme-vertical classes.

It is impossible to judge whether some part is not needed, another part reduces code duplication etc. Controversial changes are mixed with helpful ones etc.

Edit: I would like to help, but it is really a lot of work to go through the patch, look for easy-to-include stuff and commit it. You cannot expect that somebody blindly commits large patches in one go. I like these new filter buttons, the introduction of named constants, debug output for makeobj.

Edit2: Your patch is also not that much consistent: It looks like the theme-manager could make use of scr_coord, scr_rect instead of koord and KOORD_VAL?

This evening I will have some free time. If you could split your patch then I could commit the not-so-controversial parts.
Parsley, sage, rosemary, and maggikraut.

prissi

#443
Maybe some more word, since it was quite late yesterday.

The main concerns about your patch was that it did not fit the exiting code.
The changed version above is with a gui_theme_t like env_t. The drawing is all in simgraph, where all the drawing using image interna imho belongs.

kierongreen

QuoteI just don't understand why you can't just apply my full patch and have the community to have a look and say what they think before you start to throw away my work.
Because generally code should only go into trunk that will stay in trunk. In this case we are all agreed that themes are a good idea - it's the implementation details that are being disputed.

QuoteAre you always censoring everyone's code or is it only mine
Censor is a strong word - everyone's patches have been reviewed with some parts accepted other parts rejected. I've had patches outright rejected (slope images for vehicles), others that have been heavily modified (initial underground view), and some that have just been incorporated as I wrote them (powerline bridges).

Ters

I have to agree that patches should not be applied to trunk before it has been reviewed. This is especially important for Simutrans, where there is no stable branch, only trunk.

kierongreen

Or at least non-trivial patches. Patches which fix a reported bug and only affect a small area of code can be applied straight away I would say.

Ters

Quote from: kierongreen on October 17, 2013, 03:20:47 PM
Or at least non-trivial patches. Patches which fix a reported bug and only affect a small area of code can be applied straight away I would say.

I agree, I was just in a bit of a hurry at the time and failed to make that distinction.

Max-Max

Quote from: Dwachs on October 17, 2013, 06:55:29 AMMax-Max, it would REALLY help your case, if you could split your gigantic patch into smaller, easier to review parts.
I did Submitting smaller patches before but somehow no one saw it and it was never reviewed.

To submit small patches might work well if they are just small patches, not complete new development. If I submit new code in small patches, they will be out of context and refused because Prissi don't see any use of them, even if I explain the intention of it.
If I get the opinion "The themeability of Simutrans has made no progress by this patch." on a patch that added quite a lot of features, then how can I submit small patches?

Quote from: Dwachs on October 17, 2013, 06:55:29 AMFrom looking at the patch, you have several different lines of development there: debug output for makeobj, named constants in simskin, alot of theme-stuff-* etc. The patch says nothing about the purpose of these theme-vertical classes.
I have touched the purpose of these classes in this thread. The class is used in the theme manager and if you have understood what the frame, symbol and horizontal class are doing, it can't be to hard to figure out what a vertical class is doing, or?

The vertical class is used when you want to constrain a theme element to only adjust size vertically, like a vertical scrollbar. The theme images are horizontal centred in the client area, but extended vertically from left to right.

Quote from: Dwachs on October 17, 2013, 06:55:29 AMIt is impossible to judge whether some part is not needed, another part reduces code duplication etc. Controversial changes are mixed with helpful ones etc.
This is exactly my point with submitting work in progress as small patches. I do try to explain what it does in this thread, but because people point out things I have already explained, I can't come to any other conclusion that my post isn't being read or understood.

Quote from: Dwachs on October 17, 2013, 06:55:29 AMEdit: I would like to help, but it is really a lot of work to go through the patch, look for easy-to-include stuff and commit it. You cannot expect that somebody blindly commits large patches in one go. I like these new filter buttons, the introduction of named constants, debug output for makeobj.
I would really welcome that more than just one person are reviewing the code. We all have different background and experience. If one person think a solution is bad, doesn't mean it is. There might be 10 others thinking it is a good solution.

Quote from: Dwachs on October 17, 2013, 06:55:29 AMEdit2: Your patch is also not that much consistent: It looks like the theme-manager could make use of scr_coord, scr_rect instead of koord and KOORD_VAL?
I do use the new scr_coord.h functions if I know I can do it without complications. The code you refer to has been moved and collected in one place. I didn't change the KOORD_VAL and koord yet because it might have introduced complications and even a larger patch.

Quote from: Dwachs on October 17, 2013, 06:55:29 AMThis evening I will have some free time. If you could split your patch then I could commit the not-so-controversial parts.
I would love too, but my local trunk is quite out of synch from the HEAD version. Maybe I can break out the makeobj DEBUG update.



Quote from: prissi on October 17, 2013, 08:50:56 AM
Maybe some more word, since it was quite late yesterday.

The main concerns about your patch was that it did not fit the exiting code.
In what way? You are very often vague in your critique. When I comment on it and ask questions I usually don't get an answer. I usually have to guess what you mean and so far I have been not so successful. But I try my best to read your mind, so here I will try again.

Before creating each patch I apply my changes to the server's HEAD revision, compile and test. Then I create the patch from there. The patch should be able to be applied to the HEAD revision without any conflicts at all.

...and here are some more cryptic comments.
Quote from: prissi on October 17, 2013, 08:50:56 AMThe changed version above is with a gui_theme_t like env_t.
What do you mean by this? I have not moved anything from env_t into gui_theme_t...

Quote from: prissi on October 17, 2013, 08:50:56 AMThe drawing is all in simgraph, where all the drawing using image interna imho belongs.
This is again an example of a comment that makes me believe that you don't understand how the code works. The theme manager isn't doing any drawing, it is calling the low level functions in simgraph for drawing.

Simgraph, as I understand it, is where we keep the drawing interface, the low level pixel routines that will be implemented differently depending on the target system. To put everything that has even a remotely connection to drawing means that even if it is a high level drawing function, it needs to be maintained for each target implementation.

some of us, in the community, has expressed the need for a canvas. This would also make better use of the drawing routines. I expressed in one of my posts that the theme manager has a good candidate for this in it's raw_bitmap_t. I also put in a comment in the code that this might be better of as a separate class.

Regarding to your own logic, it doesn't make sense to break out code, used privately by one class, into an already very large C structural module. I thought we where going to more OOP, not more structural C.

Now, the theme elements are using the display_image routines it is only the preparation of the theme images that is done internally because there is no way to use the display_image routines on something else than the screen. This is why I brought up the canvas concept earlier...



Quote from: kierongreen on October 17, 2013, 10:31:49 AMBecause generally code should only go into trunk that will stay in trunk. In this case we are all agreed that themes are a good idea - it's the implementation details that are being disputed.
I find this strange because you guys can submit code that breaks functionality, unreviewed and uncriticized. While my code can be refused for the wrong formatting of spaces and tabs. It seems like there is one set of rules for you guys and a much stricter, non forgiving set of rules for every one else.

Quote from: kierongreen on October 17, 2013, 10:31:49 AMCensor is a strong word - everyone's patches have been reviewed with some parts accepted other parts rejected. I've had patches outright rejected (slope images for vehicles), others that have been heavily modified (initial underground view), and some that have just been incorporated as I wrote them (powerline bridges).
But it is what it is. Ideas and solution that isn't in line with what one single person agree on, is just taken out or rewritten, without first consulting the author or discuss it. How can this not be censorship?
- My code doesn't have bugs. It develops random features...

Dwachs

Here is a quick review of your patch. As you see you cannot expect anybody to just take it and commit.
-- macros.h: these changes should go into related gui and simstring headers
-- simhalt.cc, koord.h: non-sensical changes
-- simsys.h, translator.h: we are using chdir not _chdir
-- simwerkz-dialogs.h: one line commented out - the dialog is broken ?
-- bild_besch.h why add all the (void) parameter lists
-- skin_writer.cc: I doubt that it would work correct if some images are not defined in the dat
-- scr_coord.h: what is purpose of the typedef tag_size {} size; construct ?
-- new files are doubled in the patch

This patch contains the following subpatches (beside the gui and theme stuff)
-- new resource and icon file
-- different debug output for makeobj
-- use ltrim for image keys
-- comments in bild_besch.h

It does not apply on trunk anymore.

Edit: It does not even compile with r6781, which is the revision the patch seems to be taken against.

In file included from gui/loadsave_frame.h:12:0,
                 from gui/banner.cc:22:
gui/savegame_frame.h:41:19: error: '_MAX_PATH' was not declared in this scope
gui/savegame_frame.h:42:25: error: '_MAX_PATH' was not declared in this scope
gui/savegame_frame.h: In constructor 'savegame_frame_t::dir_entry_t::dir_entry_t(button_t*, button_t*, gui_label_t*, savegame_frame_t::dirlist_item_t, const char*)':
gui/savegame_frame.h:78:19: warning: 'savegame_frame_t::dir_entry_t::button' will be initialized after [-Wreorder]
gui/savegame_frame.h:77:19: warning:   'button_t* savegame_frame_t::dir_entry_t::del' [-Wreorder]
gui/savegame_frame.h:69:3: warning:   when initialized here [-Wreorder]
gui/savegame_frame.h: At global scope:
gui/savegame_frame.h:109:15: warning: unused parameter 'fullpath' [-Wunused-parameter]
gui/savegame_frame.h:111:15: warning: unused parameter 'fullpath' [-Wunused-parameter]

Now guess who is pissed as well :(
Parsley, sage, rosemary, and maggikraut.

Markohs

#450
I'm having it a look too, to the ThemeWorked.zip

I don't know who typed this, if Max-Max of prissi, but I see no point in:

* why adding (void) to functions without parameters? I don't see the point.
* macros.h adds a lot of empty lines for no reason too.

Well, appart from that, that's minor things that well, should just be fixed, I don't see much problems, but I wonder why there are so many uppercase macros in new code:


            image_id const img = sel-- != 0 ? button_t::pos_button_normal : button_t::pos_button_pushed;
            display_color_img(img, offset.x + 2, offset.y, 0, false, true);


To:



            display_img_aligned( skinverwaltung_t::posbutton->get_bild_nr(SKIN_BUTTON_POS + (sel == 0) ), scr_rect( offset.x, offset.y, 14, LINESPACE ), ALIGN_CENTER_V | ALIGN_CENTER_H, true );


New code should be easier to read, and it's not.

But well, the patch is too big for me to figure if there is something wrong.

prissi

The uppercase marcros are for once toget rid of #if MULTI_THREAD #else. The functions coudl be shorter if gui_theme_t remberes all the images. I can easily add this.

I think there is some time needed to cool this off. I will take a short time out (especially since my cold got worse).  I have also still to go over the final patch, as it was rather a quick work in progress. I would like to implement also the 3x3 image buttons (which I think is a very good idea of Max-Max) and woudl certainly answer to your comments.

But how to proceed now:

- If there are no objections, I would like to submit the cleaned up scrollbar.cc since the one in trunk is really confusing and lots of compatibility code from the time it was even a C-file. I think this is rather independent of any patch.

- During the work I saw that some dialogue use roundbuttons, but declared them as buttons. That was not so obvious until the themes (and especially since now both buttons are skinable), but needs to be fixed. I will do this on trunk, it need to be fixed regardless of how the theme manager is implemented.

- Furthermore, apart for the implementation of the theme manager, most changes from Max-Max are in both patches and, when concerning dialogues, are not really very controversial. If ok, I can commit them, so the decision is left between the way the images for the theme manager are loaded (and also much smaller patches to look at).

kierongreen

QuoteIf I get the opinion "The themeability of Simutrans has made no progress by this patch." on a patch that added quite a lot of features, then how can I submit small patches?
I actually understand where you are coming from with regards to this. With the updated landscapes I ended up having to submit a huge patch in the end for this reason. Small patches are more likely to work there way into trunk (because they are quicker to check) but as you say - not much point doing this if you need a large patch to introduce something useful.

QuoteI would love too, but my local trunk is quite out of synch from the HEAD version. Maybe I can break out the makeobj DEBUG update.
If you are submitting patches you need to ensure your local trunk is kept up to date. Yes this is a pain, however it is necessary - otherwise people reviewing the patch will have great difficulty doing so.

QuoteI find this strange because you guys can submit code that breaks functionality, unreviewed and uncriticized. While my code can be refused for the wrong formatting of spaces and tabs. It seems like there is one set of rules for you guys and a much stricter, non forgiving set of rules for every one else.
Firstly we are all human - we make mistakes from time to time. Sometimes someone will have tested a patch extensively on Linux only for it to break on Windows (or vice versa). Sometimes we submit patches quickly - if there's a known bug and there is a simple fix it would be silly to have to wait for code to be reviewed. However when we are adding a major feature then it is discussed first and reviewed by other people. Themes are clearly a major feature - one that I support being introduced and it's good that through discussion here the best overall solution will be incorporated into trunk. What I'm not going to do is take sides on your preferred implementation vs prissi's.



Overall I'll repeat comments I made earlier - if you've got an idea then write the code. By all means share code along the way but make sure it does something that adds real functionality to simutrans before requesting it get added to trunk. That way if you decide to make huge disruptive changes at least people will understand there is a need for these.

Max-Max

Quote from: Dwachs on October 17, 2013, 06:12:26 PM
Here is a quick review of your patch. As you see you cannot expect anybody to just take it and commit.

-- macros.h: these changes should go into related gui and simstring headers
I guess you are refering to to D_GET_CENTER_ALIGN_OFFSET(), D_GETFAR_ALIGN_OFFSET and GET_THEME_PICTURE.
Well they are all macros and the file is called macro.h no comments of what to expect in it. If you don't want people to put macros in a file called macro.h you have to put in some information about it. I can agree that GET_THEME_PICTURE shouldn't be there and I did move it after the patch was submitted.

I really had no clue of where to put these std::string functions, because it isn't a part of the simstring functions only dealing with char*. In this post I was raising the question of where to place them.

But at this point it seems a bit odd to put them in macro.h, I'm more than willing to admit this. But where would I but std::string related functions?

-- simhalt.cc, koord.h: non-sensical changes
What specific are you referring too?

-- simsys.h, translator.h: we are using chdir not _chdir
Check this information.
First I used _chdir() and then changed it back to chdir() because I was convinced that it would only give more fire to the refusal discussions and I was right. I must honestly have missed a place...

-- simwerkz-dialogs.h: one line commented out - the dialog is broken ?
No the dialogue works fine, but at the time I was debugging and suspected I had two instances of the same dialogue.
When I look back at it now I realize this is to fire the dialogue from the toolbar. A simple comments in the beginning of the file would have made things easier to understand for someone new to the code.
This is what code review is all about :thumbsup:

-- bild_besch.h why add all the (void) parameter lists
A common misconception for C programmers, is to assume that a function prototyped as follows takes no arguments:
int foo();
In fact, this function is deemed to take an unknown number of arguments. Using the keyword void within the brackets is the correct way to tell the compiler that the function takes NO arguments.

Google it or check a few links.
Is there a difference between foo(void) and foo() in C++ or C
int foo (int argc, ...) vs int foo() vs int foo(void) in C

I learned to always use foo(void) over foo(), unless you really need to have different implementations of foo with different arguments. foo(void) makes it absolutely clear, 100% that there will not be any parameters what so ever. So I have the habit of filling in the void when I see an foo(). I didn't know this would be a problem worth mentioning to refuse a patch...

-- skin_writer.cc: I doubt that it would work correct if some images are not defined in the dat
Ahh, you are right. My intention was to have makeobj to automatically fill in empty nodes for the first 100 indexes. But then I made the DEBUG switch so we can see what goes wrong instead of guessing if we miss to define empty image indexes. I forgot to restore it.
Again, this is what code review is about :thumbsup:

-- scr_coord.h: what is purpose of the typedef tag_size {} size; construct ?
Have you tried to declare a struct referencing itself? The use of tag_ concept is a well known solution to this very problem.
A struct is exactly same thing as a class with all members public but by tradition structs are used for type-like "classes".
The use of typedef struct tag_size allows us to define a constructor to the yet not defined type. The main advantage of a constructor in a struct is to initialize it. For some reason Prissi don't like that a struct or class initialises itself at creation, hence his concept of invalidate the type, which introduces more problems than it solves. It indicates that the type in deed can be invalid and ther fore should be checked every time before use.

I have no problem with making this to a class as long all members are public, as I mentioned in an earlier post.

-- new files are doubled in the patch
I only used TortoiseSVN menu option "Create patch...". If code has been duplicated I have no idea why...

QuoteThis patch contains the following subpatches (beside the gui and theme stuff)
-- new resource and icon file
-- different debug output for makeobj
-- use ltrim for image keys
-- comments in bild_besch.h
I submitted the icon patch earlier and then again together with the original patch[/url].
Well, maybe but when developing, things goes in parallel. It's not like I pick to do one of those thing one after another. I probably could have created some of it in individual patches, but I'm not sure how isolated these things can be done as patches when it is already implemented. If I had tried to break it up and something goes wrong, we would have all this discussion again that it's **** and shouldn't have been submitted at all.

QuoteIt does not apply on trunk anymore.
This is probably because it has gone quite some time now since I posted the patch. I have no control over how fast it is implemented into the trunk, so you can't really blame me for it.

QuoteEdit: It does not even compile with r6781, which is the revision the patch seems to be taken against.
In file included from gui/loadsave_frame.h:12:0,
                 from gui/banner.cc:22:
gui/savegame_frame.h:41:19: error: '_MAX_PATH' was not declared in this scope
gui/savegame_frame.h:42:25: error: '_MAX_PATH' was not declared in this scope
gui/savegame_frame.h: In constructor 'savegame_frame_t::dir_entry_t::dir_entry_t(button_t*, button_t*, gui_label_t*, savegame_frame_t::dirlist_item_t, const char*)':
gui/savegame_frame.h:78:19: warning: 'savegame_frame_t::dir_entry_t::button' will be initialized after [-Wreorder]
gui/savegame_frame.h:77:19: warning:   'button_t* savegame_frame_t::dir_entry_t::del' [-Wreorder]
gui/savegame_frame.h:69:3: warning:   when initialized here [-Wreorder]
gui/savegame_frame.h: At global scope:
gui/savegame_frame.h:109:15: warning: unused parameter 'fullpath' [-Wunused-parameter]
gui/savegame_frame.h:111:15: warning: unused parameter 'fullpath' [-Wunused-parameter]
I reverted to rev 6781, downloaded and dropped in the patch I posted. No problems at all applying the patch. Compiled, no errors...
I seriously hope you don't think I submit code that doesn't compile in my end?

On MSDN we can read.
"The C Runtime supports path lengths up to 32768 characters in length, but it is up to the operating system, specifically the file system, to support these longer paths. The sum of the fields should not exceed _MAX_PATH for full backwards compatibility with FAT32 file systems. Windows 2000, Windows XP Home Edition, Windows XP Professional, Windows Server 2003, Windows Server 2003, and Windows Vista NTFS file system supports paths up to 32768 characters in length, but only when using the Unicode APIs. When using long path names, prefix the path with the characters \\?\ and use the Unicode versions of the C Runtime functions."

Maybe this is a Microsoft thing? If this doesn't work on Linux, we need a way of defining the MAX_PATH and MAX_FILENAME that works for both windows, mac, linux and unicode.

QuoteNow guess who is pissed as well :(
For what?
It is my work that has been dissected, rewritten, called ugly, useless, pointless, unnecessary, confusing. It is I who has repeated the same information over and over again and still it doesn't seems like no one is reading it. Unless you are upset about this too, but I doubt you are ;)

-------------------------

Quote from: Markohs on October 17, 2013, 07:07:41 PMI'm having it a look too, to the ThemeWorked.zip
This is Prissi's rewrite of my original code. My original patch can be found here.

Quote from: Markohs on October 17, 2013, 07:07:41 PM
I don't know who typed this, if Max-Max of prissi, but I see no point in:

* why adding (void) to functions without parameters? I don't see the point.
* macros.h adds a lot of empty lines for no reason too.
See my answer above about the foo(void) vs foo().
The extra empty lines are present in Prissi's patch, not mine.

Quote from: Markohs on October 17, 2013, 07:07:41 PM
Well, appart from that, that's minor things that well, should just be fixed, I don't see much problems, but I wonder why there are so many uppercase macros in new code:


            image_id const img = sel-- != 0 ? button_t::pos_button_normal : button_t::pos_button_pushed;
            display_color_img(img, offset.x + 2, offset.y, 0, false, true);


To:



            display_img_aligned( skinverwaltung_t::posbutton->get_bild_nr(SKIN_BUTTON_POS + (sel == 0) ), scr_rect( offset.x, offset.y, 14, LINESPACE ), ALIGN_CENTER_V | ALIGN_CENTER_H, true );
This is again Prissi's rewritten version of my code. display_img_aligned() isn't coming from my patch at all.
When it comes to capital names, I was again told my code was "dead ugly" when enums wasn't all in capitals.

Quote from: Markohs on October 17, 2013, 07:07:41 PM
New code should be easier to read, and it's not.
But well, the patch is too big for me to figure if there is something wrong.
I agree and this is a very subjective definition of what is readable code? But from the patch name you stated, I can only assume you have been looking into Prissi's version of my original code.

-------------------------

Quote from: prissi on October 17, 2013, 07:46:32 PM
The uppercase marcros are for once toget rid of #if MULTI_THREAD #else. The functions coudl be shorter if gui_theme_t remberes all the images. I can easily add this.
Prissi, if you want to be an active part in developing this, we have to work together. We can't start to implement things in different directions just because you don't like my Object Oriented approach. I'm working towards a goal and if you keep changing my structure all the time and add stuff that interfere with what I'm developing it will be nothing of this.

The reason why the theme manager is adding all the images to the global image list is because the display_xxx routines can't work with anything else. I first had this code in the theme_element_t to directly draw the images to the screen, but I was told to use the structure we already have. My goal is to remove this dependency, but it is to early to start messing around with it.

Quote from: prissi on October 17, 2013, 07:46:32 PMI think there is some time needed to cool this off. I will take a short time out (especially since my cold got worse).  I have also still to go over the final patch, as it was rather a quick work in progress. I would like to implement also the 3x3 image buttons (which I think is a very good idea of Max-Max) and woudl certainly answer to your comments.
...and why can't you just implement the theme manager, theme_elements as they are? Why do you have to rewrite this code from an object oriented design to more or less structural C design? Why is it so hard for you to accept an object oriented design?

Quote from: prissi on October 17, 2013, 07:46:32 PM
But how to proceed now:

- If there are no objections, I would like to submit the cleaned up scrollbar.cc since the one in trunk is really confusing and lots of compatibility code from the time it was even a C-file. I think this is rather independent of any patch.

- During the work I saw that some dialogue use roundbuttons, but declared them as buttons. That was not so obvious until the themes (and especially since now both buttons are skinable), but needs to be fixed. I will do this on trunk, it need to be fixed regardless of how the theme manager is implemented.

- Furthermore, apart for the implementation of the theme manager, most changes from Max-Max are in both patches and, when concerning dialogues, are not really very controversial. If ok, I can commit them, so the decision is left between the way the images for the theme manager are loaded (and also much smaller patches to look at).
I have repeatedly times explained both that this is work in progress, it is to early to change the way images are loaded. I also have the feeling that you will do this your way without discussing it, and if we where, my planned implementation will not be respected anyway.

If you want to help me it is fine, but please ask me first what you can do. Please respect that I'm using a more object oriented approach to this, that I have a vision and a goal I'm working towards.

It is like building a house and when the basement is done, some one comes and builds a parking lot over it so then I have to redesign the whole house again, and again, and again...

-------------------------

Quote from: kierongreen on October 17, 2013, 07:52:26 PM
I actually understand where you are coming from with regards to this. With the updated landscapes I ended up having to submit a huge patch in the end for this reason. Small patches are more likely to work there way into trunk (because they are quicker to check) but as you say - not much point doing this if you need a large patch to introduce something useful.
Exactly my point. I have been trying to tell every one this from the very beginning.

Quote from: kierongreen on October 17, 2013, 07:52:26 PM
If you are submitting patches you need to ensure your local trunk is kept up to date. Yes this is a pain, however it is necessary - otherwise people reviewing the patch will have great difficulty doing so.
My patches are in synch, compiled and tested with the HEAD revision when I submit them. Then depending on how much Prissi rewrites it, I get a true hell when it is in the trunk because TortoiseSVN get confused when code marked as new already exists in a different form.

Quote from: kierongreen on October 17, 2013, 07:52:26 PM
Firstly we are all human - we make mistakes from time to time. Sometimes someone will have tested a patch extensively on Linux only for it to break on Windows (or vice versa). Sometimes we submit patches quickly - if there's a known bug and there is a simple fix it would be silly to have to wait for code to be reviewed. However when we are adding a major feature then it is discussed first and reviewed by other people. Themes are clearly a major feature - one that I support being introduced and it's good that through discussion here the best overall solution will be incorporated into trunk.
I have no problem with this procedure, but as it is now, Prissi rewrites my code without so much as asking what it does and what the intention is.

Quote from: kierongreen on October 17, 2013, 07:52:26 PMWhat I'm not going to do is take sides on your preferred implementation vs prissi's.
You don't need to take a "side", you can have your own opinion on what approach you think is a good one. You can comment on pros and cons, no need to take a "side".

Quote from: kierongreen on October 17, 2013, 07:52:26 PMOverall I'll repeat comments I made earlier - if you've got an idea then write the code. By all means share code along the way but make sure it does something that adds real functionality to simutrans before requesting it get added to trunk. That way if you decide to make huge disruptive changes at least people will understand there is a need for these.
I guess I have fundamentally misunderstood how the trunk is supposed to be used. I have seen code go in that doesn't work or is still in progress. I made the assumption that I could commit parts of my code that was still in development.

The very problem is that it is impossible to commit small patches that does something. This is new development and there are many pieces that works together. I can't simply submit a patch with a full theme system AND make it small.

I get double messages, make smaller patches, you need to include more into the patch so we understand the context. It doesn't matter what I do, I will get **** for it in the end no matter what I do.


-------------------------

I know I sound very pessimistic, but I wasn't when I started this. I became very defensive and pessimistic when all my hard work isn't respected and instead of asking and let me explain how things are planned, code is just discredited and rewritten. As soon I realized that Prissi already was rewriting it to something else, I lost interest... What is the point when everything I do is discredited, rewritten and not respecting that I happen to have a more object oriented implementation in mind. When my work in progress isn't seen as work in progress, but as unnecessary code not doing anything useful.
When I give the artists more options to work with, I get to here the opposite, that I put in constrains and the artists has less freedom. Things about how my implementation works that isn't true.

Maybe I should just go underground as so many other developers had done before me (I wonder why) and develop this in silence...
- My code doesn't have bugs. It develops random features...

Ters

Some general advice, as Max-Max is not the only one to do some of the things pointed out as problematic with his patch:

       
  • When working on several unrelated things, use different working directories to keep changes for each separate. An alternate solution for when one needs to do a small change, like a bug fix, and only a working directory full of changes is available, is to shelve all changes (in a patch or whatever fancy functionality may be available for this), revert, do the fix, commit and then unselve again. Shelving in a patch won't work if the change involves moving files, or so it seems someone here found out regarding translation patches.
  • Always look through the diff before submitting something, whether it is by committing directly to VCS or by posting a patch. It's not foolproof, but I sometimes find lingering debug things and unrelated changes that way.
  • Patches should preferrably be small and contain only a specific feature. Sometimes, changesets are big no matter what, and I can also see how big patches are needed in order for reviewers to see the big picture. (This also applies to changesets committed directly to VCS, although I am a bit guilty of letting some sometimes unrelated code cleanup free ride on other changesets. My excuse is that it avoids extra CI builds, which can take quite some time at work.)