wx update and cmake #12

ghost · 2016-02-21T14:28:03Z

This is progress for updating wx and I also have a cmake. It doesn't build yet but I've got it down to some linker errors.

Not ready yet.

ghost · 2016-02-21T15:07:17Z

I've upgraded to the latest version of CMarkup for now until I or someone else makes the switch to tinyxml.

The Markup.cpp and Markup.h files should be placed in the code/Torsion directory before cmake configuration and generation.

dottools · 2016-02-21T18:10:11Z

code/Torsion/PreCompiler.cpp

@@ -411,10 +411,10 @@ void PreCompilerThread::UpdateOutput( const wxString& path, wxFileOffset& lastRe
      return;

   wxString buffer;
-   wxChar* log = buffer.GetWriteBuf( size );
+   wxChar* log = new wxChar(buffer.GetChar(size));


This is wrong. wxChar is a typedef depending on unicode setting.

I'm aware that a lot of the stuff is wrong. Don't worry I'll get to them.

Note this is more than just a slightly wrong implementation. It is completely the wrong code... like not even heading in the right direction. It makes zero sense to the intent of the original code.

The original code was right and just needs to be ported correctly. Any other change here is a waste of time.

Oh wow. I'm sure glad you mentioned that. I was under the impression that some crap I slapped in there to get it to compile might actually WORK! Phew. good thing you let me know. That was a close one.

@blackwc - Look... we are trying to help you. That is all.

The obvious way to make things just compile is commenting it out. Replacing it with random code that does something completely different from the original makes other coders think you have no clue what you are doing. It looks like inexperience... not a work around.

I'll make note of your quirk for replacing working code with broken code and wait for you to finish.

However cut out the attitude. No one here is your enemy or attacking you.

I apologize if me warning you that your code is wrong set you off. I've never seen any coder replace code with nonsense and even with your warning I wanted to be sure you knew you're heading in the wrong direction.

This is my project. I will write my opinion on pull requests. I won't be right at times and misunderstandings occur when communicating here. I hope you understand that moving forward and don't take things I write as an insult.

What set me off is that there was no reason to pile on.

What I did was no different than setting a bunch of variables to zero except it was quicker and allowed me to efficiently get all the includes in order so it would compile. I marked all instances with "TODO ?" to signify where I put the placeholder code.

What I did was efficient for the task I set out to do which was get every thing in order so it would at least compile and also leave notations so people would know where the nonfunctional placeholder code is.

It was my fault for PRing a branch too soon in development and allowing over eager first-time repo-managers a chance to dig around in a very rough draft.

I'm done with it but I'm leaving the branch up.

I agree I am over eager, but I may have more repo management experience then you think. :)

Hah... thanks. My fingers do most of my spelling for me. :)

dottools · 2016-02-21T18:17:24Z

There's so many things wrong with this PR I'm not going to bother commenting each and every little thing. So many operations are commented out entirely instead of fixed, and so on. PRs should only be done when they're actually completed. Now don't get me wrong there's a huge amount of great work on getting it compliant with the latest wxWidgets version. However it's still very much an incomplete mess.

ghost · 2016-02-21T18:19:13Z

Who said PRs are only for completed works? Is that in the github bible? This PR is tracking my branch. That's what it's for. I'll rebase later.

tomspilman · 2016-02-21T19:01:28Z

Hi @blackwc.

I think the wxWidgets port work is great... but i will not merge CMake or CMarkup.

I want to have VS solutions and projects in this repo and not a CMake build script. When I discussed build scripting in #4 it was simply about the glue between the steps and not project generation. It was mean for replacing this:

https://github.com/SickheadGames/Torsion/blob/master/msbuild.proj

Not for automating project generation.

As For CMarkup I omitted it from the repo on purpose. The license in the zip clearly stipulates it is evaluation purposes only. We're not evaluating it... I did that 10 years ago... it is great and i'd love to use it, but it is not open source licensed. I wrote #1 specifically for these reasons. That said if you want to reach out to the CMarkup developers and ask about using it in open source that would be great.

Keeping a WIP PR like this is fine... it doesn't bother anyone and lets us see progress. Just in the end lets rebase and flatten some of the commits.

ghost · 2016-02-21T19:05:45Z

Understood. I'll look into it.

tomspilman · 2016-02-21T19:14:44Z

code/Torsion/AutoCompText.cpp

-   m_Data.UngetWriteBuf( len );
+
+   wxChar* result = new wxChar(m_Data.GetChar( len + 1 )); // never allow zero!
+   //m_Data.UngetWriteBuf( len );


Note this change is wrong.... it does something completely illogical and will not work.

Note that if memory serves me Torsion was not written for Unicode or wide character support. If you are trying to port to the latest wxWidgets and at the same time upgrade to wchar or unicode you are just making things more difficult.

Stick to wxWigdets with wxUSE_UNICODE set to zero to avoid all this for now.

I'm aware of the senseless lines. I just wanted to put torsion on legs, however shaky, for the time being. I am going to attempt to move to unicode as well, yes (maybe, we'll see).

I am going to attempt to move to unicode as well

I recommend not doing that for this PR. Simply move to wxWidgets ansii.

The move to unicode is a totally separate issue and we will deal with it, but just not now.

tomspilman · 2016-02-21T21:01:53Z

code/Torsion/tsComboBox.cpp

@@ -8,6 +8,6 @@

 void tsComboBox::EnableReselection()
 {
-   m_selectionOld = -1;
+   //m_selectionOld = -1;


I'm not sure what the fix is for this one... we will have to look more closely at the old wxWidgets code and the new code and figure it out.

ghost · 2016-02-21T21:40:29Z

Torsion now gives me a bad taste in my mouth. I'm closing this PR but leaving the branch up.

tomspilman · 2016-02-21T21:43:20Z

@blackwc - That is fine. Good luck with your projects.

cmake and wx update progress

4b00a13

theblooopz added 2 commits February 21, 2016 10:11

CMakeLists.txt typo

6c962fc

got it to compile

ce4de65

dottools reviewed Feb 21, 2016
View reviewed changes

tomspilman reviewed Feb 21, 2016
View reviewed changes

ghost closed this Feb 21, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wx update and cmake #12

wx update and cmake #12

ghost commented Feb 21, 2016

ghost commented Feb 21, 2016

dottools Feb 21, 2016

ghost Feb 21, 2016

tomspilman Feb 21, 2016

ghost Feb 21, 2016

tomspilman Feb 21, 2016

tomspilman Feb 21, 2016

ghost Feb 21, 2016

tomspilman Feb 21, 2016

ghost Feb 21, 2016

tomspilman Feb 21, 2016

dottools commented Feb 21, 2016

ghost commented Feb 21, 2016

tomspilman commented Feb 21, 2016

ghost commented Feb 21, 2016

tomspilman Feb 21, 2016

ghost Feb 21, 2016

tomspilman Feb 21, 2016

tomspilman Feb 21, 2016

ghost commented Feb 21, 2016

tomspilman commented Feb 21, 2016

wx update and cmake #12

wx update and cmake #12

Conversation

ghost commented Feb 21, 2016

ghost commented Feb 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dottools commented Feb 21, 2016

ghost commented Feb 21, 2016

tomspilman commented Feb 21, 2016

ghost commented Feb 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 21, 2016

tomspilman commented Feb 21, 2016