search

sphair / ClanLib

ClanLib is a cross platform C++ toolkit library.
Other
344 stars 76 forks source link

Properly implement DPR into clan::InputDevice. #57

Closed keigen-shu closed 9 years ago

keigen-shu commented 9 years ago

The unit used for mouse positioning was switched from Device Dependent Pixel (hardware pixel unit) to Device Independent Pixel (DPI-scaled pixel unit) in the DPR patch on January. This commit brings back DDP positioning for pointing devices and adds DIP positioning into clan::InputDevice as separate functions.

Note that the UI API has not changed at all; the module still uses DIP unit on everything. PointerEvent's pos() and set_pos() uses the DIP unit.

P/S: Implemented for Linux and Windows; only tested on Linux. I also removed Doxygen comments and added override keyword on the member functions of InputDeviceProvider implementors

rombust commented 9 years ago

I like this. Much clearer It has the additional bonus benefit for myself, when writing directly to the GraphicContext (avoiding Canvas) - It is now clear what type of mouse coordinates InputEvent/InputDevice returns.

dpjudas commented 9 years ago

I like this patch except for the fact that the default functions (get_position, etc) return DDP pixels rather than DIP. It really should be the other way around. get_position() should return a DIP float while a get_ddp_position() style function should return the integer variant.

Since there seems to be a lot of disagreement between you, me and Rombust about this, I wrote a small article about why DDP positioning shouldn't be the default:

http://esoteric.clanlib.org/~mbn/physical-pixel/

rombust commented 9 years ago

dpjudas, that is a good article that you have written. I now fully understand.

It is going to take me a while to get used to it. But that's the problem with me being an old style programmer (as the "old dog new tricks" saying goes)!

keigen-shu commented 9 years ago

I understand. I used the old convention to follow what GraphicContext does (get_size() is DDP), but I guess the reason behind that is different than this. I'll close this PR and make a new one without the merge conflict.