Pr/simplify derivations #805

uli42 · 2019-05-13T20:05:04Z

I want to know if this has chances to get merged, from a license point of view. It it okay to handle derived code like I do here?

sunweaver

@uli42: Thanks for working on this massive clean-up branch. You mainly have two strategies:

renaming the Xserver's original function to xorg_<bla> and calling the original code from within the DDX's code (plus run some extra code before or after calling the Xserver original code): This is ok license wise.
Secondly, you move snippets from hw/nxagent/NX*.c to dix/*.c: This is not ok, as you mix GPL-2 code and Xserver code (licensed under MIT/X11 or some such). What is possible: put the code fragment from hw/nxagent/NX*.c into some helper function and call that helper function from within the DIX code base. This is a formalistic trick for calling nxagent code paths from within the DIX code base. (The function call comes from you, so you can license it under the Xserver's license).

nx-X11/programs/Xserver/Xext/xvdisp.c

nx-X11/programs/Xserver/Xext/shm.c

nx-X11/programs/Xserver/dix/window.c

sunweaver · 2019-05-21T09:12:57Z

nx-X11/programs/Xserver/hw/nxagent/NXdispatch.c

-	free(CurrentSelections);
-    CurrentSelections = (Selection *)NULL;
-    NumCurrentSelections = 0;
+    xorg_InitSelections();


Ahhhh... now I get what you are doing.

Ok. These xorg_* namespacing commits, they will work copyright wise...

sunweaver · 2019-05-21T09:14:41Z

nx-X11/programs/Xserver/dix/extension.c

+             */
+
+            || (nxagentRenderTrap && strcmp(extensions[i]->name, "RENDER") == 0)
+#endif


Move this bit into nxagent DDX code, then it is ok.

sunweaver · 2019-05-21T09:14:59Z

nx-X11/programs/Xserver/dix/extension.c

+             */
+
+            if (nxagentRenderTrap && strcmp(extensions[i]->name, "RENDER") == 0)
+                continue;


Move to DDX code base.

sunweaver · 2019-05-21T09:16:50Z

nx-X11/programs/Xserver/Imakefile

@@ -248,7 +248,7 @@ NXAGENTOBJS = hw/nxagent/miinitext.o     \
              hw/nxagent/NXglxext.o      \
              hw/nxagent/NXmiexpose.o    \
              hw/nxagent/NXresource.o    \
-              hw/nxagent/NXdamage.o      \
+              hw/nxagent/damage.o        \


This feels wrong (symlinks damage.[c|o] from DIX to DDX). Why not build damage.c at its original location?

It is the same as with some other linked files: NXdamage.c did not have any code left after cleanup, but in dix/damage.c there are NXAGENT_SERVER ifdefs. That define is only set during nxagent compilation, but not during dix compilation.

sunweaver · 2019-05-21T09:17:55Z

nx-X11/programs/Xserver/miext/damage/damage.c

 	if (imageblt)
 	    (*pGC->ops->ImageGlyphBlt)(pDrawable, pGC, x, y, n, charinfo,
 				       FONTGLYPHS(pGC->font));
 	else
 	    (*pGC->ops->PolyGlyphBlt)(pDrawable, pGC, x, y, n, charinfo,
 				      FONTGLYPHS(pGC->font));
+#endif


strictly thinking, this is also GPL-2 license stuff. Even if it is just an ifdef block that gets added.

I have taken another look into this and found I have made a mistake. This must be an #ifndef instead of #ifdef. I guess disabling code is ok, license wise. Right?

sunweaver · 2019-05-21T12:51:32Z

On Di 21 Mai 2019 11:53:10 CEST, Ulrich Sibiller wrote: uli42 commented on this pull request. > @@ -248,7 +248,7 @@ NXAGENTOBJS = hw/nxagent/miinitext.o \ hw/nxagent/NXglxext.o \ hw/nxagent/NXmiexpose.o \ hw/nxagent/NXresource.o \ - hw/nxagent/NXdamage.o \ + hw/nxagent/damage.o \ It is the same as with some other linked files: NXdamage.c did not have any code left after cleanup, but in dix/damage.c there are NXAGENT_SERVER ifdefs. That define is only set during nxagent compilation, but not during dix compilation.

Ok. -- DAS-NETZWERKTEAM c\o Technik- und Ökologiezentrum Eckernförde Mike Gabriel, Marienthaler str. 17, 24340 Eckernförde mobile: +49 (1520) 1976 148 landline: +49 (4354) 8390 139 GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31 mail: [email protected], http://das-netzwerkteam.de

sunweaver · 2019-05-22T14:41:31Z

Hi,

On Di 21 Mai 2019 20:21:29 CEST, Ulrich Sibiller wrote: uli42 commented on this pull request. > if (imageblt) (*pGC->ops->ImageGlyphBlt)(pDrawable, pGC, x, y, n, charinfo, FONTGLYPHS(pGC->font)); else (*pGC->ops->PolyGlyphBlt)(pDrawable, pGC, x, y, n, charinfo, FONTGLYPHS(pGC->font)); +#endif I have taken another look into this and found I have made a mistake. This must be an #ifndef instead of #ifdef. I guess disabling code is ok, license wise. Right?

Yes, disabling original Xserver code should be ok license wise. At least, let's assume that. Mike -- DAS-NETZWERKTEAM c\o Technik- und Ökologiezentrum Eckernförde Mike Gabriel, Marienthaler str. 17, 24340 Eckernförde mobile: +49 (1520) 1976 148 landline: +49 (4354) 8390 139 GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31 mail: [email protected], http://das-netzwerkteam.de

uli42 · 2019-05-22T14:45:21Z

Ok, and what about simply calling an existing function? Is that ok or do we need a wrapper, too? I am thinking about this:

#ifdef NXAGENT_SERVER
    nxagentFlushConfigureWindow();
#endif

sunweaver · 2019-05-22T15:09:55Z

Hi, Am Mittwoch, 22. Mai 2019 schrieb Ulrich Sibiller:

Ok, and what about simply calling an _existing_ function? Is that ok or do we need a wrapper, too? I am thinking about this: ``` #ifdef NXAGENT_SERVER nxagentFlushConfigureWindow(); #endif ```

/me sighs. Dogdy case. Esp. the nxagent* is not ideal. I am thinking about patch acceptance (finally) on the Xorg upstream side. So, consider... Is that call really needed? How do other hardware DDXs handle such hook calls? Mike

…

-- Gesendet von meinem Fairphone2 (powered by Sailfish OS).

uli42 · 2019-05-22T15:20:51Z

That's the problem with most of NX's changes: It's no easy to judge... Generally one could add hooks here and there. But you cannot have them everywhere without cluttering the whole upstream code.

I am tempted to drop changes that are enclosed in ifdef TEST or DEBUG. And changes that simply print something. But there are also changes the really look important...

sunweaver · 2019-05-22T15:38:46Z

Hi,

On Mi 22 Mai 2019 17:20:51 CEST, Ulrich Sibiller wrote: That's the problem with most of NX's changes: It's no easy to judge... Generally one could add hooks here and there. But you cannot have them everywhere without cluttering the whole upstream code.

Yes, I get that.

I am tempted to drop changes that are enclosed in ifdef TEST or DEBUG. And changes that simply print something.

Please do. Feel tempted.

But there are also changes the really look important...

Truely spoken. And yet, we don't know... (urgh). Mike -- DAS-NETZWERKTEAM c\o Technik- und Ökologiezentrum Eckernförde Mike Gabriel, Marienthaler str. 17, 24340 Eckernförde mobile: +49 (1520) 1976 148 landline: +49 (4354) 8390 139 GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31 mail: [email protected], http://das-netzwerkteam.de

sunweaver · 2019-06-11T10:26:15Z

@uli42: is PR #805 ready for a second review iteration? Please rebase beforehand. Thanks.

uli42 · 2019-06-11T11:23:31Z

no, currently this is not uptodate. I will alert when it's time for another review. Currently I try to pull out commits from here into separate branches to get stuff included that can clearly be merged.

sunweaver · 2019-06-11T16:06:54Z

ACK. Mike Am Dienstag, 11. Juni 2019 schrieb Ulrich Sibiller:

…

no, currently this is not uptodate. I will alert when it's time for another review. Currently I try to pull out commits from here into separate branches to get stuff included that can clearly be merged. -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #805 (comment)

-- Gesendet von meinem Fairphone2 (powered by Sailfish OS).

sunweaver · 2019-08-10T12:03:38Z

@uli42: What's the status of the PR (#805)? Is it ready for review again? I left some requests / comments. Please get in touch.

uli42 · 2019-08-10T12:35:54Z

All the current PRs are preconditions to this. Once they are merged I will go in with this one. And Once it is merged I finally can complete my 1.4.2 patch. At least that's the plan... Uli Mike Gabriel <[email protected]> schrieb am Sa., 10. Aug. 2019, 14:03:

…

@uli42 <https://github.com/uli42>: What's the status of the PR (#805 <#805>)? Is it ready for review again? I left some requests / comments. Please get in touch. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#805?email_source=notifications&email_token=ABQHBZF6WHVYE242VGMBHBLQD2VBXA5CNFSM4HMS72I2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4AMGBQ#issuecomment-520143622>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHBZGPCOMNBG7FCOIMBH3QD2VBXANCNFSM4HMS72IQ> .

"clipboard=something" does not need to be passed on, so return after setting nxagentOptions accordingly. This fixes [nx-X11/programs/Xserver/hw/nxagent/Args.c:1584]: (error) Uninitialized variable: argc

Right at the beginnigng of nxagentParseSingleOption we check for "clipboard" and prepare argv and argc accordingly for ddxProcessArgument. The removed code thus could never be reached.

This should disable clipboard but effictively did activate clipboard=both.

…Dispatch

NXxvdisp.c only exists to set/unset nxagentXvTrap before/after dispatch. There's no need to duplicate the original code. We now rename the original dispatch functions and call them in our dispatch code. Also drop check for sun and cygwin, as they never appeared in xorg upstream code.

instead of having an own (identical) copy

The only difference was one short code block. There's no need to clone ~100 lines just for that...

Our version only adds some commented code, so it is not really necessary. But it is cleaner to handle it that way.

no function change

Change ShmExtensionInit to call the unaltered upstream version and trick it to do what we want.

instead of having an identical copy in nxagent_miShmPutImage

The only difference to the dix version was a fprintf if compiled with -D TEST.

is not dependent on any root window and need only be called once on startup.

sunweaver · 2019-09-29T15:59:38Z

Ok, I guess it is time for this one, I guess?

uli42 · 2019-10-29T11:53:20Z

I have now split this up, see #858 to #869. Some NXwindow commits are left, I need to rework them because of "incompatible license" issues.

uli42 requested review from sunweaver and Ionic May 13, 2019 20:05

sunweaver requested changes May 21, 2019

View reviewed changes

uli42 force-pushed the pr/simplify_derivations branch 3 times, most recently from dbbe658 to 92eb55c Compare May 31, 2019 23:25

uli42 force-pushed the pr/simplify_derivations branch from 92eb55c to 627641e Compare June 21, 2019 14:00

uli42 force-pushed the pr/simplify_derivations branch from 627641e to 58cb1cc Compare August 6, 2019 20:03

uli42 added 11 commits September 1, 2019 13:40

Args.c: fix unitialized variable

5b5ea5e

"clipboard=something" does not need to be passed on, so return after setting nxagentOptions accordingly. This fixes [nx-X11/programs/Xserver/hw/nxagent/Args.c:1584]: (error) Uninitialized variable: argc

Args.c: remove unreachable code

31fdc04

Right at the beginnigng of nxagentParseSingleOption we check for "clipboard" and prepare argv and argc accordingly for ddxProcessArgument. The removed code thus could never be reached.

Args.c: fix clipboard=0 or -clipboard 0

3cc7209

This should disable clipboard but effictively did activate clipboard=both.

Args.c: simplify options string allocation

dcee638

NXrender.c, render.c: fix memory leaks

9ce59a6

NXrender.c: fix possible memory leak

f4ea243

NXshm.c: rework code to be identical for ProcShmDispatch and SProcShm…

1384e74

…Dispatch

NXshm.c: use upstream versions of Dispatch functions

1b7d18c

instead of having an own (identical) copy

NXwindow.c: call upstream version UnmapWindow()

36f11b3

NXwindow.c: use upstream function ReparentWindow()

3803fad

The only difference was one short code block. There's no need to clone ~100 lines just for that...

uli42 added 7 commits September 1, 2019 14:25

NXevents.c: call upstream DefineInitialRootWindow()

2668d2c

NXEvents.c: mark NX specific code

81970d4

NXevents.c: call upstream ProcAllocEvents()

baf2272

Our version only adds some commented code, so it is not really necessary. But it is cleaner to handle it that way.

NXglyphcurs.c: use dixChangeCG()

72c7b41

NXglyph.c: adapt FindGlyph() to better match render/glyph.c

09fc26f

no function change

NX{dispatch,mitrap,picture,render}.c: mark NX changes

6ac13c4

NXrender.c: modify code to match upstream as close as possible

a60f654

uli42 force-pushed the pr/simplify_derivations branch from 58cb1cc to 047ab33 Compare September 1, 2019 13:22

uli42 added 19 commits September 5, 2019 20:53

miwindow.c: mark NX changes

007502a

NXshm.c: call upstream ShmExtensionInit

1c55def

Change ShmExtensionInit to call the unaltered upstream version and trick it to do what we want.

NXshm.c: call upstream miShmPutImage

631f6b4

instead of having an identical copy in nxagent_miShmPutImage

NXrender.c: call upstream versions of (S)ProcRenderDispatch

b4b6511

Pixmap.c: call miModifyPixmapHeader twice instead of duplicating code

6801d6a

NXglyph.c: mark NX changes

b17088d

NXwindow.c: drop own UnmapWindow()

c205f2b

The only difference to the dix version was a fprintf if compiled with -D TEST.

NXwindow.c: call InitRootWindow from dix

1162081

nxagent: move atom initiatialization to Init.c

bc00ae9

is not dependent on any root window and need only be called once on startup.

nxagent: drop unused return code of nxagentRenderRealizeCursor

250eb10

Render.c: simplify nxagentRenderRealizeCursor

c175824

Render.c: drop unused nxagentValidatePicture

3541c5f

Render.c. simplify nxagentMatchingFormats

8207057

Render.c: fix broken comment

8a8c12d

Render.c: drop unused nxagentRasterizeTrapezoid

b2a23d5

NXpicture.c: call xorg upstream versions of Create*Picture

3437b25

NXrender.c: Code simplifications

0e9bca5

fixup! NX{dispatch,mitrap,picture,render}.c: mark NX changes

52aca52

fixup! NX{dispatch,mitrap,picture,render}.c: mark NX changes

6e5100a

uli42 force-pushed the pr/simplify_derivations branch from 9ee3a20 to 6e5100a Compare September 5, 2019 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr/simplify derivations #805

Pr/simplify derivations #805

uli42 commented May 13, 2019

sunweaver left a comment •

edited

Loading

sunweaver May 21, 2019

sunweaver May 21, 2019

sunweaver May 21, 2019

sunweaver May 21, 2019

uli42 May 21, 2019

sunweaver May 21, 2019

uli42 May 21, 2019

sunweaver commented May 21, 2019 via email

sunweaver commented May 22, 2019 via email

uli42 commented May 22, 2019

sunweaver commented May 22, 2019 via email

uli42 commented May 22, 2019

sunweaver commented May 22, 2019 via email

sunweaver commented Jun 11, 2019

uli42 commented Jun 11, 2019

sunweaver commented Jun 11, 2019 via email

sunweaver commented Aug 10, 2019

uli42 commented Aug 10, 2019 via email

sunweaver commented Sep 29, 2019

uli42 commented Oct 29, 2019

Pr/simplify derivations #805

Are you sure you want to change the base?

Pr/simplify derivations #805

Conversation

uli42 commented May 13, 2019

sunweaver left a comment • edited Loading

Choose a reason for hiding this comment

sunweaver May 21, 2019

Choose a reason for hiding this comment

sunweaver May 21, 2019

Choose a reason for hiding this comment

sunweaver May 21, 2019

Choose a reason for hiding this comment

sunweaver May 21, 2019

Choose a reason for hiding this comment

uli42 May 21, 2019

Choose a reason for hiding this comment

sunweaver May 21, 2019

Choose a reason for hiding this comment

uli42 May 21, 2019

Choose a reason for hiding this comment

sunweaver commented May 21, 2019 via email

sunweaver commented May 22, 2019 via email

uli42 commented May 22, 2019

sunweaver commented May 22, 2019 via email

uli42 commented May 22, 2019

sunweaver commented May 22, 2019 via email

sunweaver commented Jun 11, 2019

uli42 commented Jun 11, 2019

sunweaver commented Jun 11, 2019 via email

sunweaver commented Aug 10, 2019

uli42 commented Aug 10, 2019 via email

sunweaver commented Sep 29, 2019

uli42 commented Oct 29, 2019

sunweaver left a comment •

edited

Loading