Bug 12042

Created attachment 11162 [details] [review]
first version of the patch sent to the mailing list

This patch is the original one sent to the mailing list at http://lists.freedesktop.org/archives/xorg/2007-August/026960.html .

Created attachment 11163 [details] [review]
patch that does apply

I have edited the previous patch to make it apply on master.

Hello,

Following my reviewing of the patch, here are some comments.                               

1/ Coding style                                                                             
----------------

* I think you should try to respect to the style used in files you are modifying.

    For instance, there is not space after opening parenthesis and
    before  closing parathesis:

    /*this is bad*/
    +  if (hostx_want_screen_size( screen, &width, &height )

    /*this is good*/
    +  if (hostx_want_screen_size(screen, &width, &height)

* Lines should not be longer than 80 characters.

* You should use two space indentation, in ephyr.c, at least.
  for instance ephyrBlockSigio uses more, and there are more functions like
  that.

*  You should not use c++ style comments (//). Please, rather use /**/.


2/ Multiple screen related variables naming
--------------------------------------------

The name struct EphyrHostScreenVars is a bit misleading IMO. I believe that
structure abstracts a screen, right ? So why not calling it something like
struct EphyrScreen ? You even named them member of EphyrHostXVars that contains
the screens EphyrHostXVars::screens so I think we should easily agree on this
one :-)

I also think you should consistently refer to the EphyrHostScreenVars as
"ephyr_screens", instead of "vars" because "screen" tells more what it is about
than "vars". This is important because it helps the code to be self documented.
    
3/ More code related thoughts
-------------------------------------------
    
* In hostx_set_screen_number():
  you call hostx_set_win_title( vars->info, "" );
  and that empties the Xephyr window's title. That breaks Xephyr behaviour a
  little bit because normally Xephyr windows have the tile: "( ctrl+shift grabs
  mouse and keyboard )". And that title changes when mouse/keyboard is grabed.
  So setting the title to "" breaks that behaviour. 
  
* in hostx_screen_init():
  In think now that the function takes a KdScreenInfo* in parameters, you should
  remove the other paramters from it as the other parameters can be retrieved
  from KdScreenInfo*. So hostx_screen_init() should take only one parameter:
  that KdScreenInfo*

* this is more of a note, but maybe we could make hostx_paint_rect() and
  hostx_paint_debug_rect() could take a struct Rect {int x1, x2, y1, y2;}
  in parameter instead of so  much parameters.

(In reply to comment #2)
> Created an attachment (id=11163) [details]
> patch that does apply
> 
> I have edited the previous patch to make it apply on master.
> 

Sorry, I forgot to say here that the patch did not apply to master because it was corrupted. The patch sent to the mailing list cannot be applied because I think the mailer of the sender wraps long lines and thus corrupts patches.

Created attachment 11813 [details] [review]
cleaned the patch according to my observations

I edited the patch slightly for:

* better legibility and style conformance with the rest of the code

* create only one card and set all the screens to the same card. The initial patch was creating several card,s each of them having only one screen. That was confusing because all the screens were having the same screen number, which was 0.

* make sure we are not obliged to provide a -screen WIDTHxSIZE option to Xephyr.  When no -screen option is provided, Xephyr just creates one screen as it was doing before. The initial patch was requiring the presence of a -screen option. Xephyr would not even launch if no -screen option was provided.

I will wait a little bit to expect a review for #11813 and will commit it into master. That patch seems to be working okay for me now.

Created attachment 11837 [details] [review]
fix a screen number bug in the xephyr windows title

Fixed a problem I introduced. The xephyr windows titles were not showing the right screen numbers.

Created attachment 11853 [details] [review]
cleanup further

Put back full title display, but try do it properly this time.

Okay testing the patch further showed that windows that are sent to the second screen (or any screen but the first one) don't receive mouse events.

It seems the bug is in the server core code.

I have entered bug #12650 to track it.

Okay I finally pushed the patch to the xserver master branch.
Commit is e5e6514ffa0fd132e0cc1b15b94119e6e8755f43.

This should close the bug.