Created attachment 230737 [details] [review]
Proposed patch

Review of attachment 230737 [details] [review]:

Could we not fill the fake device with bogus data instead? I'd rather not have test app specific changes in code that's going to run in production.

well we /could/ but stricly speaking the function gsd_wacom_device_get_area() /can/ return NULL so we might want to check that before using the returned data IMHO, as this could affect not just test-wacom.

gsd_wacom_device_get_area() would return NULL if:

 - The device cannot be opened
 - The property cannot be retrieved

Another way to do this is:
- add a -DFAKE_AREA to the test-wacom CPPFLAGS
- return fake data based on the current monitor setup in gsd-wacom-device.c in an #ifdef FAKE_AREA case.
- add a warning and exit cleanly from the function when the gsd_wacom_device_get_area() call fails.

Looking into it.

Created attachment 240577 [details] [review]
Prevent failure when it cannot get the device's area and also use fake data

Hi,

I am proposing a patch that hopefully meets Bastien's suggestions.

Created attachment 240578 [details] [review]
Show mouse pointer if -DFAKE_AREA is set

I am also proposing this patch which prevents hiding the cursor when -DFAKE_AREA is set because it is very convenient when one is testing without the proper hardware.

Review of attachment 240577 [details] [review]:

::: panels/wacom/cc-wacom-page.c
@@ +279,3 @@
+		device_cal = g_new0 (int, 4);
+		device_cal[0] = 0;
+		device_cal[1] = gdk_screen_get_width (screen);

That won't work on multi-monitor setup (I know it's just for testing, but...).

@@ +286,3 @@
+#endif /* FAKE_AREA */
+
+		if (device_cal == NULL) {

You can move that inside the ifdef as well.

Review of attachment 240577 [details] [review]:

::: panels/wacom/cc-wacom-page.c
@@ +23,3 @@
 #include <config.h>
 
+#ifdef FAKE_AREA

Where is this defined?

Review of attachment 240578 [details] [review]:

::: panels/wacom/calibrator/gui_gtk.c
@@ +495,3 @@
 	gdk_window_set_background_rgba (window, &black);
 
+#ifndef FAKE_AREA

Where is this defined?

Maintainer change

Created attachment 240725 [details] [review]
New patch to use a fake area

I am attaching a new version of the previous prevention patch.
This one encloses the code I had left out of the #ifdef according to Bastien's comments.

I have changed the macro approach and now the macro has a value and is by default "turned off". I did so because I am propagating that macro to the calibrator so it shows the cursor (in a new patch that will follow this one) and if we turn it on by default, then (as far as I understand) it will always show the cursor.

If you have a better approach please let me know because my autotools-fu could be better.

Created attachment 240726 [details] [review]
Updated patch to show the cursor when we are testing with a fake area

As promised in the previous comment. I am attaching a new version of the calibrator patch (see previous comments).

Review of attachment 240726 [details] [review]:

::: panels/wacom/calibrator/Makefile.am
@@ +17,3 @@
 	gui_gtk.h
 
+libwacom_calibrator_la_CPPFLAGS = -DUSE_FAKE_AREA=$(USE_FAKE_AREA)

Where you do get $(USE_FAKE_AREA) from?

Sorry for the late reply (had some filters in GMail that were hiding all the bugzilla stuff from me).

That macro comes from the Makefile in the wacom folder above.

This is done in the calibrator only to show the pointer but probably it would be a better idea to check which device is performing the movement and hide/show the pointer accordingly.

Review of attachment 240726 [details] [review]:

This patch needs to be folded into the first.

Review of attachment 240725 [details] [review]:

::: panels/wacom/Makefile.am
@@ +58,3 @@
+# Set USE_FAKE_AREA to 1 in order to use a fake area
+# based on the current screen for the calibration
+USE_FAKE_AREA = 0

That's always going to set USE_FAKE_AREA to 0.

@@ +59,3 @@
+# based on the current screen for the calibration
+USE_FAKE_AREA = 0
+export USE_FAKE_AREA

I'm certain this isn't going to work. This isn't a bash script.

You can't compile one library with 2 different sets of defines.

You'll need to compile the calibrator once with the USE_FAKE_AREA define set, once without.

Variables defined in a Makefile.am also aren't inherited in child directories' Makefiles.

(In reply to comment #17)
> Review of attachment 240725 [details] [review]:
> @@ +59,3 @@
> +# based on the current screen for the calibration
> +USE_FAKE_AREA = 0
> +export USE_FAKE_AREA
> 
> I'm certain this isn't going to work. This isn't a bash script.

It actually works. My intention was that developers could change that 0 to 1, make clean on that dir and make again (I have just tried it and it works).

I will try to have it compiled twice as I got a new idea of how to do this.

Created attachment 241345 [details] [review]
New patch for compiling the calibrator twice

Hi Bastien,

Please check out these changes that compile the calibrator twice but still have the macro defined in the old way I did.

Created attachment 241349 [details] [review]
New patch which doesn't use a value for the macro

New version without defining a value for the macro.

Created attachment 241350 [details] [review]
New version for showing the cursor in the calibrator

Here's the new version of the second patch according to Bastien's suggestions.

Review of attachment 241349 [details] [review]:

::: panels/wacom/Makefile.am
@@ +57,3 @@
 
+test_wacom_CPPFLAGS = $(INCLUDES) -DFAKE_AREA
+test_wacom_LDADD = $(PANEL_LIBS) $(WACOM_PANEL_LIBS) $(builddir)/calibrator/libwacom-calibrator-test.la

Where's the corresponding changes to the calibrator sub-dir?

Review of attachment 241349 [details] [review]:

::: panels/wacom/Makefile.am
@@ +57,3 @@
 
+test_wacom_CPPFLAGS = $(INCLUDES) -DFAKE_AREA
+test_wacom_LDADD = $(PANEL_LIBS) $(WACOM_PANEL_LIBS) $(builddir)/calibrator/libwacom-calibrator-test.la

I see it's in the other patch.

Review of attachment 241350 [details] [review]:

::: panels/wacom/calibrator/Makefile.am
@@ +20,3 @@
 libwacom_calibrator_la_LDFLAGS = $(PANEL_LDFLAGS)
 
+libwacom_calibrator_test_la_SOURCES =	\

You'll need to split out the creation of a test library from the functional change.

Hi Bastien,

Could elaborate a bit more? Do you mean that the creation of this lib should come in an independent patch (I guess not)?

Thanks in advance,

(In reply to comment #26)
> Could elaborate a bit more? Do you mean that the creation of this lib should
> come in an independent patch (I guess not)?

You guessed wrong ;)

Created attachment 241631 [details] [review]
Create new testing lib

:) very well, I personally would do it as I had it before but mentioning it in the log message (which was missing).

I am attaching two patches that split it like you said.

Created attachment 241632 [details] [review]
Show the cursor in the calibrator UI

... and the isolated FAKE_AREA in the calibrator.

Review of attachment 241349 [details] [review]:

Ready to commit

Review of attachment 241631 [details] [review]:

Looks good other than that.

::: panels/wacom/calibrator/Makefile.am
@@ +20,3 @@
 libwacom_calibrator_la_LDFLAGS = $(PANEL_LDFLAGS)
 
+libwacom_calibrator_test_la_SOURCES =	\

On one line is fine.

Review of attachment 241632 [details] [review]:

Fine.

Removed the newline. Pushed to master.