Add missing UI functions for X11 Window class #580
Conversation
fullset
added
improvement
src/ui/x11/x11_ui.cpp
Outdated
| } | ||
|
|
||
| for ( size_t i = 0u; i < _lines.size(); ++i ) { | ||
| const Point2d & start = std::get<0>( _lines[i] ); |
There was a problem hiding this comment.
I'm thinking to combine the code with /src/ui/win/win_ui.h file where we use structures to store data instead of tuples. Reading such code is very difficult so we might need to use the same idea in this class.
There was a problem hiding this comment.
We could move structures from win_ui.h file into src/ui/ui.h. The only thing missing is we need an integer value instead of 3 uint8_t values for color.
There was a problem hiding this comment.
We could do the same as it's done for Win32: just calculate RGB value during drawing (add a function at the top of the file to do such).
There was a problem hiding this comment.
Ok, it sounds good. I'll do this.
|
Hi @fullset let's finish this pull request :) |
| @@ -26,7 +30,64 @@ class UiWindowX11 : public UiWindow | |||
| uint32_t _width; | |||
| uint32_t _height; | |||
|
|
|||
| std::vector< std::pair<Point2d, uint32_t> > _point; | |||
| struct PointToDraw | |||
There was a problem hiding this comment.
Hi @fullset could we move these identical structures in a common location as src/ui/ui.h file to avoid code duplication?
There was a problem hiding this comment.
As I see, there are 2 structures PointToDraw and LineToDraw which are almost identical in x11 and win_ui. And there are no problem to combine them.
But EllipseToDraw structures in x11 and win_ui have different semantics. In win_ui we specify top, left, right and bottom coordinates but it x11 we specify top, left coordinates and width and height. I could make common constructor of 4 doubles but I'm not sure that it's right decision. What do you think?
There was a problem hiding this comment.
Let's stick to one design which should have top-left coordinate and width with height. For Windows during drawing we could easily find right and bottom values by adding width and height.
There was a problem hiding this comment.
Hi, @ihhub.
It looks like I've broken windows building by my last changes but I can't find the reason because I haven't windows PC.
There was a problem hiding this comment.
Could you put structures back in ui/ui.h but don't declare these variables:
std::vector<PointToDraw> _point;
std::vector<LineToDraw> _lines;
std::vector<EllipseToDraw> _ellipses;?
src/ui/x11/x11_ui.cpp
Outdated
| void UiWindowX11::drawEllipse( const Point2d & center, double xRadius, double yRadius, const PaintColor & color ) | ||
| { | ||
| // XDrawArc needs x and y coordinates of the upper-left corner of the bounding rectangle but not the center of the ellipse. | ||
| Point2d position( center.x - xRadius, center.y - yRadius ); |
fullset
force-pushed
the
issue-459
branch
2 times, most recently
from
5cd6bab to
607fde4
Compare
Closes #459
@ihhub
I've added 3 methods to
UiWindowX11class. I've made them similar todrawPoint()method.Also, I've slightly changed
drawPoint()method to draw the cross(x).Review please as you'll have a time.