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

Win32a changes submitted #6

Closed
wants to merge 659 commits into from
Closed

Win32a changes submitted #6

wants to merge 659 commits into from

Conversation

@Bill-Gray
Copy link
Contributor

@Bill-Gray Bill-Gray commented Jan 13, 2016

With this, "mainstream" PDCurses should be updated to include the new Win32a flavor. Other flavors are not greatly affected, except as described at http://projectpluto.com/win32a.htm#2015may31 and in the commit comments. So I think this will be straightforward, even though it's a fair bit of code.

@TheGentzy
Copy link

@TheGentzy TheGentzy commented Mar 23, 2016

I've been using this recently and its been running smoothly. I did have a few issues getting it to work along the way, but I believe those were my mistakes. I really do hope this gets merged with the main repo.

@techtonik
Copy link
Contributor

@techtonik techtonik commented Mar 23, 2016

This needs to be rebased to solve conflicts.

@luke-jr
Copy link

@luke-jr luke-jr commented Mar 23, 2016

Merged, not rebased. Rebasing is considered a bad practice.

curses.h Outdated
#if defined( CHTYPE_32)
#define CHTYPE_LONG 1 /* chtypes will be 32 bits */
#elif !defined( CHTYPE_16)
#define CHTYPE_LONG 2 /* chtypes will be (default) 64 bits */

This comment has been minimized.

@luke-jr

luke-jr Mar 23, 2016

What if CHTYPE_16 is defined?

This comment has been minimized.

@Bill-Gray

Bill-Gray Mar 23, 2016
Author Contributor

Hi Luke,

On 2016-03-23 00:26, Luke-Jr wrote:

In curses.h #6 (comment):

@@ -34,11 +34,16 @@ PDCurses portable platform definitions list:
#define XOPEN 1 /* X/Open Curses routines /
#define SYSVcurses 1 /
System V Curses routines /
#define BSDcurses 1 /
BSD Curses routines /
-#define CHTYPE_LONG 1 /
size of chtype; long */
+#if defined( CHTYPE_32)

  • #define CHTYPE_LONG 1 /* chtypes will be 32 bits */
    +#elif !defined( CHTYPE_16)
  • #define CHTYPE_LONG 2 /* chtypes will be (default) 64 bits */

What if CHTYPE_16 is defined?

In that case, CHTYPE_LONG is left undefined.

There's some discussion of this in curses.h (search for "Originally,
PDCurses used a short" and read from there.) Until Win32a came along,
your options were to leave CHTYPE_LONG undefined, in which case you
got 16-bit chtypes and some limited attributes and colors; or you
could #define CHTYPE_LONG 1 and get 32-bit chtypes, basic Unicode
support (i.e., up to U+FFFF), and plenty of colors and attributes.

Since this was a binary decision, it made sense to have the options
be "either CHTYPE_LONG is defined, or it isn't." Making it a ternary
choice, with 64-bit chtypes as an option, threw a spanner into the
works. For backward compatibility, I just set things up so that
option was enabled with #define CHTYPE_LONG 2.

-- Bill


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/wmcbrine/PDCurses/pull/6/files/c1ab1e40dc35a905edd65e5263ce9d7029d5e452#r57108353

curses.h Outdated
# if _LP64
typedef unsigned int chtype;
# else
typedef unsigned long chtype; /* 16-bit attr + 16-bit char */

This comment has been minimized.

@luke-jr

luke-jr Mar 23, 2016

You're including stdint.h, so why not use uint32_t?

This comment has been minimized.

@Bill-Gray

Bill-Gray Mar 23, 2016
Author Contributor

Hi Luke,

On 2016-03-23 00:27, Luke-Jr wrote:

In curses.h #6 (comment):

@@ -83,11 +88,15 @@ extern "C"
typedef unsigned char bool; /* PDCurses Boolean type */

#ifdef CHTYPE_LONG
-# if _LP64
-typedef unsigned int chtype;
-# else
-typedef unsigned long chtype; /* 16-bit attr + 16-bit char */
-# endif

  • #if(CHTYPE_LONG >= 2) /* "non-standard" 64-bit chtypes */
  •    typedef uint64_t chtype;
    
  • #else /* "Standard" CHTYPE_LONG case, 32-bit: */
  •    # if _LP64
    
  •        typedef unsigned int chtype;
    
  •    # else
    
  •        typedef unsigned long chtype;  /\* 16-bit attr + 16-bit char */
    

You're including stdint.h, so why not use uint32_t?

Um.  Looks to me as if you have a good point there.  I'd prefer letting

the folks who write 'stdint.h' puzzle out which size is which. Figuring
out what size various types are isn't as simple as checking _LP64; there
are different macros to check on different compilers, and there's a reason
'stdint.h' is often a complicated mess. It could be we have people out
there thinking they're getting 32-bit chtypes when they're really getting
64-bit chtypes.

Similarly,  a few lines further down :

typedef unsigned short chtype; /* 8-bit attr + 8-bit char */

should,  I think,  use uint16_t.  I'll make that change.

(I didn't do this before,  due to tunnel vision.  I was adding my code

for 64-bit chtypes and didn't look too closely at the existing code
for 32-bit and 16-bit chtypes.)

-- Bill


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/wmcbrine/PDCurses/pull/6/files/c1ab1e40dc35a905edd65e5263ce9d7029d5e452#r57108369

@luke-jr
Copy link

@luke-jr luke-jr commented Mar 23, 2016

BTW, is it possible to build this such that the console vs GDI decision can be made at runtime (eg, via a command-line option)?

@techtonik
Copy link
Contributor

@techtonik techtonik commented Mar 23, 2016

Merged, not rebased. Rebasing is considered a bad practice.

I'd say that merges and huge commits are hard to review, so rebasing and splitting changes into small independent PRs is often the only way forward. Depends on reviewer, though.

@techtonik
Copy link
Contributor

@techtonik techtonik commented Mar 23, 2016

BTW, is it possible to build this such that the console vs GDI decision can be made at runtime (eg, via a command-line option)?

Technically it is possible, but I am not sure that it is a good fit for this PR.

@Bill-Gray
Copy link
Contributor Author

@Bill-Gray Bill-Gray commented Mar 23, 2016

Hi Luke,

On 2016-03-23 00:31, Luke-Jr wrote:

BTW, is it possible to build this such that the console vs GDI decision can be made at
runtime (eg, via a command-line option)?

I suppose it could be done, with a great deal of revision. (A lot
of revision.) There's also not a lot of duplicate code involved, so
whichever flavor you chose, you'd be loading in a lot of code for the
other flavor without using it. I guess I'd have to ask "why would you
want to do this?"

Note that SDL would also be a choice, so you could load up all
three sets of code and make a runtime decision amongst them. (There
is actually apt to be more overlap between SDL, GDI, and X11 than
between any of them and Win32 console. All three flavors will use
similar logic for blinking, determining background/foreground
colors, and a few other items. All three could support user and
programmatic resizing, under/over/left/right/strikeout lining,
"real" blinking, and dimmed, italic, and bold text. Win32
console can't support most of these, and has little code overlap
with those three flavors.)


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#6 (comment)

@Bill-Gray
Copy link
Contributor Author

@Bill-Gray Bill-Gray commented Mar 23, 2016

On 2016-03-23 04:54, anatoly techtonik wrote:

Merged, not rebased. Rebasing is considered a bad practice.

I'd say that merges and huge commits are hard to review, so rebasing and splitting
changes into small independent PRs is often the only way forward. Depends on reviewer,
though.

You're both getting beyond my very limited knowledge of git (only
started to use it recently, though I'm now quite enthusiastic about it).
A search on "git merge vs. rebase" is helping to lessen my ignorance.

The "Golden Rule of Rebasing", described here,

https://www.atlassian.com/git/tutorials/merging-vs-rebasing/conceptual-overview

suggests some possible problems (I think) with rebasing.

Dunno if this would help, but... since the time I forked from
wmcbrine/PDCurses/master, that branch has had three commits. Two
("warning suppression" and "Parameters not given here", on 7 Feb)
are now incorporated in Bill-Gray/PDCurses/master. The remaining
commit, pulling in Laura Michaels' TTF/Unicode SDL changes, would
be easy enough for me to add in.

If I do that, wmcbrine/PDCurses/master could be rolled back
to where it was when I forked it, and the merger/rebasing would be
done on the basis of "here are some changes from Bill-Gray/PDCurses/master,
and there has been no activity on wmcbrine/PDCurses/master since this
version was forked."

Seems to me this ought to simplify the problem, but as a Git
newbie, I may be overlooking something.

-- Bill


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#6 (comment)

@luke-jr
Copy link

@luke-jr luke-jr commented Mar 23, 2016

The purpose of runtime selection would be to avoid opening a separate window when invoked from a command prompt.

@Bill-Gray Bill-Gray force-pushed the Bill-Gray:master branch from 36303a1 to 2e7d5ce Mar 27, 2016
@techtonik
Copy link
Contributor

@techtonik techtonik commented Aug 11, 2016

@Bill-Gray I would try to split it into several small logical changes using:

git branch smallchange   # create new branch from where you are
git checkout smallchange # switch to new branch
git rebase -i HEAD~50     # edit last 50 commits on new branch, remove extra, squash

If you fork smallchange branch from you current master, all removed commits will still be available from your `master.

@Bill-Gray
Copy link
Contributor Author

@Bill-Gray Bill-Gray commented Aug 11, 2016

Hi Anatoly,

There are several problems here. Some of them may seem troublesome
to me only due to my lack of Git knowledge, I hope. The most basic
problem is uncertainty as to whether these changes are welcome in
wmcbrine/PDCurses in the first place.

I know that William has some reservations about the proposed changes
in Win32a. I'm not entirely sure what they are. It may well be that
we will stay forked, or that William would like some of the changes
but not others. I'd prefer to combine our efforts, but should make
it clear that I'm okay with it if these changes are rejected. I've
drawn on updates submitted to wmcbrine/PDCurses, such as Laura Michaels'
improvements for SDL and Mark Hessling's X11 improvements, and expect
to continue to do so.

I'm not very interested in splitting things up until I know which,
if any, of the Win32a modifications might actually make it into the
"official" PDCurses. With that out of the way :

The changes I've submitted begin with a huge dose of changes made
from March 2010 to January 2016 that predate the move to GitHub. These
are only documented on my page at

http://www.projectpluto.com/win32a.htm

which is nearly useless for GitHub purposes. Ideally, I'd have been
using GitHub back then, and all the various changes tracked on that page
would be much more readily tracked via GitHub. But I didn't do that.
Those changes appear as a massive blob of a commit on 2016 January 13,
"initial Win32a version added". It would be hard to break those up,
after the fact, into a series of commits (lovely though that would be).
Following the move to GitHub, though, the commits are a series of
small logical changes. (I actually was able to separate out some of the
2010 to 2016 work as smaller commits, though not as much as one would
ideally wish.)

Since I submitted this pull request back in January, by the way, I've
made several dozen commits to Bill-Gray/PDCurses. If there is actually
some desire to incorporate my changes into wmcbrine/PDCurses, I'd
be submitting a new pull request that is somewhat more up to date
than the original one.

-- Bill

On 2016-08-11 05:58, anatoly techtonik wrote:

@Bill-Gray https://github.com/Bill-Gray I would try to split it into several small
logical changes using:

|git branch smallchange # create new branch from where you are git checkout smallchange #
switch to new branch git rebase -i HEAD~50 # edit last 50 commits on new branch, remove
extra split |

If you fork |smallchange| branch from you current master, all removed commits will still
be available from your `master.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6 (comment), or mute the thread
https://github.com/notifications/unsubscribe-auth/AP6BrkJ4DmLDVLXi2FQTT4DR-z3LqRE7ks5qevJMgaJpZM4HEZFA.

@techtonik
Copy link
Contributor

@techtonik techtonik commented Oct 24, 2016

Hi @Bill-Gray. This will never be merged unless somebody helps you to split this huge piece of work into separate small pull requests - one feature per request. It is very hard to review big pieces of code when people have only got about 15 minutes of free time on their side project such as PDCurses during the day (or even week). I spent 5 from 15 already by answering. =)

@Bill-Gray
Copy link
Contributor Author

@Bill-Gray Bill-Gray commented Oct 25, 2016

On 2016-10-24 04:32, anatoly techtonik wrote:

Hi @Bill-Gray https://github.com/Bill-Gray. This will never be merged
unless somebody helps you to split this huge piece of work into separate
small pull requests - one feature per request. It is very hard to review
big pieces of code when people have only got about 15 minutes of free
time on their side project such as PDCurses during the day (or even week).
I spent 5 from 15 already by answering. =)

I'm pretty much proceeding under the assumption that it won't get
merged, and that the fork is a permanent one.

I also don't see this as a big problem. We will effectively have
"classic" PDCurses at wmcbrine/PDCurses, which doesn't change much except
for occasional bug fixes. And we'll have "new and improved" PDCurses at
Bill-Gray/PDCurses, with the Windows GDI flavor and assorted changes.

I perhaps should have pushed harder for a merge back in March 2010
when this fork was in its infancy. Had I done that, and/or if I'd been
using Git so that there would be a series of small commits rather than
the massive blob of a commit dropped on William last January, then a
merge might be more likely. (Or, at least, a merge of some of the
features of this fork.)

But at this point, breaking up the changes between March 2010 (when
Win32a started) and January 2016 (when I moved to GitHub) would be a huge
project and might not get pulled in anyway. (Or only certain bits and
pieces would get pulled, and we'd have two versions anyway.)

Unless I hear objections, I'll withdraw this pull request and the
two forks can go their separate ways in peace.

-- Bill

@wmcbrine
Copy link
Owner

@wmcbrine wmcbrine commented Jan 16, 2018

Although I've taken many of the other changes here into my tree, I have yet to include the actual "Win32a" port, because I would've had to dumb it down to fit it into the intentionally limited framework of 3.5 (in particular, no 64-bit chtype, nor anything that depends on it), only to build it up again later. But I do plan to include it in a future version (soon), that more radically revamps PDCurses.

@Bill-Gray
Copy link
Contributor Author

@Bill-Gray Bill-Gray commented Jan 17, 2018

Glad to hear this is apt to happen, and I agree with your strategy of getting other bits, such as 64-bit chtypes, in place first (and you've been getting some pieces of the puzzle into place: bold/italic fonts, real blinking, etc.)

As best I can tell, the main obstacle to merging the Win32a port and associated changes is that it's a significant pain in the anatomy. Nothing that's rocket surgery, but it's a lot of changes. I don't think there are major philosophical differences to be bridged (and I'm quite flexible on minor philosophical differences).

If you see changes I could make in my fork that would simplify the process, please let me know. (One that I've started on is applying some of your changes, where possible/appropriate, to my fork, thereby making them at least slightly less "different". There's quite a bit more that I can do there, though.)

@GitMensch
Copy link
Contributor

@GitMensch GitMensch commented Apr 4, 2018

@Bill-Gray Can you please give a short update about "back-merging changes of this repo to your fork"? I think it would be good to do so (and provide a new release in the other repo to verify that all the changes work before more approaches are done to merge the two repos).

@wmcbrine Can you please give a short update/details about 64bit chtype addition and other changes you see necessary before '(fully) merging the Bill-Gray fork (especially win32a)?

@Bill-Gray
Copy link
Contributor Author

@Bill-Gray Bill-Gray commented Apr 5, 2018

@GitMensch : not entirely sure what you mean by this...

I've been pleased to see a lot of interesting things in @wmcbrine's updates in the last few months, and have gotten around to putting some of his fixes and improvements into my fork. I was rather impressed that William found a way to get blinking text and 256 colors in the Win32 console. I'd be grabbing still more of his improvements, but I've some contract work that has been taking most of my time for a while now. I'm also just glad to see that "official" PDCurses has some life to it.

Unfortunately, we've now diverged a lot. Previously, I'd kept up with William's changes, so that my fork was essentially "official" PDCurses plus changes. A merger now would be painful, though not impossible, if William is so inclined. It would probably start with adding 64-bit chtypes. Doing that would not really be very difficult. I think that at that point, the Win32a code could simply be slurped in. Since "official" PDCurses doesn't have Win32a at all, there would be no issue with conflicts with existing code.

After that, we could put dimmed, overlined, blinking, struck-out, RGB, full Unicode, etc. capabilities into other platforms that can support them (basically X11 and SDL1 and 2). That would take some more serious effort, but shouldn't be rocket surgery, especially since all of it does already work in the forked version. But again, I've no idea if William is interested in this, and this is his repo.

Bill-Gray added 5 commits Dec 24, 2018
…t least 'sort of', though further effort is apt to be needed
…, the key is _not_ 'already handled', and marking that flag will cause the next character to be treated as if _it_ had already been handled.
@GitMensch

This comment has been minimized.

Copy link
Contributor

@GitMensch GitMensch commented on vt/pdcdisp.c in 4d0d4f5 Dec 27, 2018

wprintf in line 264?

This comment has been minimized.

Copy link
Contributor Author

@Bill-Gray Bill-Gray replied Dec 27, 2018

(And it'd be needed for 266, too.)
Not really needed, unless the format string itself contains wide chars (which in this case, it doesn't; it's an ASCII format specifier that is specifying that a wide character is to be printed). If I wanted to print out a string in Russian, I might use
wprintf( L"фаваылпаы\n");
Or I could use

wchar_t *text_to_print = L"фаваылпаы";

printf( "%ls\n", text_to_print);
Bill-Gray added 25 commits Feb 7, 2020
…ot 64-bit chtypes and more than 16 colors. In that case, you get the square of the number of colors (i.e., each pair of colors can be a color pair), up to INT_MAX or 20 bits worth of color pairs. This really should be determined at compile time, but I didn't see a way of doing that.
… for pointing this out). Reinstated INFOEX=N flag.
…curses or PDCurses drawing). Should have been added in to previous commit.
…fork of PDCurses, applies here as well).
…at it follows ncurses practice of returning the maximum mouse interval. I don't see the value of returning ERR. Also followed Simon Sobisch's comment that the maximum interval ought to be a defined constant and greater than 1000.
…if you've called them before 'initscr()', you're doing something wrong and would like to know about it.
…ill some work to do before it runs correctly (will require ncurses' direct-colors model). Realized some 'break' statements were missing (to no ill effect, but still... they should be there.)
…lled mid-write. (Something I only noticed with 'picsview' expanded to a huge screen and with lots of resizing.) You have to notice that not all bytes were written, check to make sure that a signal handler really was the problem, then try again until all bytes do get written.
…than or equal to 16. Replaced the GNU Unifont-based font (which is non-public-domain) with a public-domain 8 x 14 VGA font.
@Bill-Gray
Copy link
Contributor Author

@Bill-Gray Bill-Gray commented Feb 27, 2020

I'm assuming this is essentially a fossil pull request, and am closing it.

@Bill-Gray Bill-Gray closed this Feb 27, 2020
@GitMensch

This comment has been minimized.

Copy link
Collaborator

@GitMensch GitMensch commented on 4ee573a May 14, 2020

@Bill-Gray can you please try to reference some git hashes from wcbrine in a "follow the lead" commit message? This would be useful for a quick reference.

This comment has been minimized.

Copy link
Owner Author

@Bill-Gray Bill-Gray replied May 15, 2020

Should have referred to wmcbrine commit 9634e66, from 2019 Jan 5. I'd already modified other makefiles to follow that commit; e.g., 8e60c57 (wherein I simply adopted William's WinCon makefile for VC) and 682e9ed (similar for SDL2). But William's fork lacks makefiles (or anything else) for WinGUI or VT; I had to modify those myself. Hence, the above commit.
William's fork also lacks support for CMake, but I didn't catch that, resulting in issue #121.

@GitMensch

This comment has been minimized.

Copy link
Collaborator

@GitMensch GitMensch commented on b5d92bf Jun 16, 2020

@Bill-Gray is this really something that can be supported that way as long as init_pair, color_pair and friends only get a short integer (which is necessary as those are the standard curses signatures)?

This comment has been minimized.

Copy link
Owner Author

@Bill-Gray Bill-Gray replied Jun 21, 2020

You do have to use init_extended_pair', init_extended_color, extended_color_content, and extended_pair_content`. These were added to ncurses a while back, and added to this fork of PDCurses in early 2020.

If you don't use those functions, you're stuck with the short-int limits, which will work as before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.