From 08b46e501c954a8c82f94793cc91b451b99a3f85 Mon Sep 17 00:00:00 2001 From: Taylor Bockman Date: Sat, 17 Aug 2019 12:56:15 -0700 Subject: [PATCH] Change point state to use a singleton to smooth out some weird bugs --- clusterview/mode_handlers.py | 41 ++++++++++++++----------------- clusterview/opengl_widget.py | 53 ++++++++-------------------------------- clusterview/point_list_widget.py | 52 +++++++++++++++++++++++++++++++++++++++ clusterview/point_manager.py | 7 ++++++ clusterview/points.py | 3 +++ main_window.py | 6 +++++ tests/test_point.py | 14 +++++------ tests/test_point_set.py | 23 ++++++++++------- 8 files changed, 118 insertions(+), 81 deletions(-) create mode 100644 clusterview/point_list_widget.py create mode 100644 clusterview/point_manager.py diff --git a/clusterview/mode_handlers.py b/clusterview/mode_handlers.py index f912827..97bef83 100644 --- a/clusterview/mode_handlers.py +++ b/clusterview/mode_handlers.py @@ -5,10 +5,11 @@ from PyQt5.QtCore import QEvent, Qt from .exceptions import ExceededWindowBoundsError from .mode import Mode from .opengl_widget import (get_bb_bottom_right, get_bb_top_left, - set_current_points, set_drawing_event, - set_move_bb_top_left, set_move_bb_bottom_right, - reset_move_bbs, viewport_height, viewport_width) + set_drawing_event, set_move_bb_top_left, + set_move_bb_bottom_right, reset_move_bbs, + viewport_height, viewport_width) from .points import PointSet +from .point_manager import PointManager class __ClickFlag: @@ -32,9 +33,10 @@ class __ClickFlag: # Size of point for drawing __POINT_SIZE = 8 -# There are a lot of module-global variables being used because of the -# nature of state management in OpenGL. -__point_set = PointSet(__POINT_SIZE, viewport_height(), viewport_width()) +# PointManager is a class that is filled with static methods +# designed for managing state. +PointManager.point_set = PointSet(__POINT_SIZE, viewport_height(), + viewport_width()) # Module level flag for left click events (used to detect a left # click hold drag) @@ -55,9 +57,11 @@ def __refresh_point_list(ctx): # it using the current __point_set. ctx.point_list_widget.clear() - for p in __point_set.points: + for p in PointManager.point_set.points: ctx.point_list_widget.addItem("({}, {})".format(p.x, p.y)) + ctx.point_list_widget.update() + def __handle_add_point(ctx, event): """ Event handler for the add point mode. @@ -69,23 +73,21 @@ def __handle_add_point(ctx, event): @param ctx A context handle to the main window. @param event The click event. """ - global __point_set - if (event.button() == Qt.LeftButton and event.type() == QEvent.MouseButtonPress): # At this point we can be sure resize_gl has been called # at least once, so set the viewport properties of the # point set so it knows the canvas bounds. - __point_set.viewport_width = viewport_width() - __point_set.viewport_height = viewport_height() + PointManager.point_set.viewport_width = viewport_width() + PointManager.point_set.viewport_height = viewport_height() # Clear any existing selections - __point_set.clear_selection() + PointManager.point_set.clear_selection() try: # No attribute at the moment. - __point_set.add_point(event.x(), event.y()) + PointManager.point_set.add_point(event.x(), event.y()) except ExceededWindowBoundsError: # The user tried to place a point whos edges would be # on the outside of the window. We will just ignore it. @@ -95,8 +97,6 @@ def __handle_add_point(ctx, event): set_drawing_event(event) - set_current_points(__point_set) - ctx.opengl_widget.update() ctx.point_list_widget.update() @@ -112,9 +112,7 @@ def __handle_edit_point(ctx, event): # # Should move the associated point in the list to the new location if # applicable. - global __point_set - - __point_set.clear_selection() + PointManager.point_set.clear_selection() # Store old x, y from event set_drawing_event(event) @@ -143,7 +141,7 @@ def ogl_keypress_handler(ctx, event): __mouse_start = None __left_click_flag = __ClickFlag.NONE - __point_set.clear_selection() + PointManager.point_set.clear_selection() reset_move_bbs() elif ctx.mode is not Mode.OFF: @@ -162,7 +160,6 @@ def __handle_move_points(ctx, event): global __left_click_flag global __mouse_start - global __point_set set_drawing_event(event) @@ -196,7 +193,7 @@ def __handle_move_points(ctx, event): dx = __mouse_start[0] - event.x() dy = __mouse_start[1] - event.y() - for p in __point_set.points: + for p in PointManager.point_set.points: if p.selected: # Use the deltas to decide what direction to move. # We only want to move in small unit increments. @@ -239,7 +236,7 @@ def __handle_delete_point(ctx, event): set_drawing_event(event) - __point_set.remove_point(event.x(), event.y()) + PointManager.point_set.remove_point(event.x(), event.y()) __refresh_point_list(ctx) diff --git a/clusterview/opengl_widget.py b/clusterview/opengl_widget.py index c972e05..b6af2ca 100644 --- a/clusterview/opengl_widget.py +++ b/clusterview/opengl_widget.py @@ -21,7 +21,7 @@ from OpenGL.GL import (glBegin, glClearColor, glColor3f, glEnable, from .exceptions import handle_exceptions, InvalidModeError, InvalidStateError from .mode import Mode -from .points import PointSet +from .point_manager import PointManager class Color(Enum): BLUE = 0 @@ -55,10 +55,8 @@ __move_bb_bottom_right = None # function local. __current_mode = None __current_event = None -__current_points = None __current_context = None - def set_drawing_context(ctx): """ Sets the drawing context so that drawing functions can properly @@ -68,26 +66,6 @@ def set_drawing_context(ctx): __current_context = ctx -def set_current_points(points): - """ - Sets the point state variable that will be passed in to the paint_gl - function, and further to the point painter functions in order to - render the scene. - - Each time a point is added, removed, or edited this point set must be - updated. - - @param points The PointSet representing the current scene. - """ - global __current_points - - if not isinstance(points, PointSet): - raise ValueError("set_current_points must recieve a PointSet as its " + - "argument.") - - __current_points = points - - def set_drawing_mode(mode): """ State management function. It is useful to look at the @@ -128,7 +106,6 @@ def set_drawing_event(event): if event is not None: __current_event = event - def set_move_bb_top_left(x, y): """ Called to set the move bounding box's top left corner. @@ -140,7 +117,6 @@ def set_move_bb_top_left(x, y): __move_bb_top_left = (x, y) - def set_move_bb_bottom_right(x, y): """ Called to set the move bounding box's bottom right corner. @@ -152,15 +128,12 @@ def set_move_bb_bottom_right(x, y): __move_bb_bottom_right = (x, y) - def get_bb_top_left(): return __move_bb_top_left - def get_bb_bottom_right(): return __move_bb_bottom_right - def reset_move_bbs(): global __move_bb_top_left global __move_bb_bottom_right @@ -185,7 +158,6 @@ def resize_gl(w, h): """ global __WIDTH global __HEIGHT - global __current_points __WIDTH = __current_context.width() __HEIGHT = __current_context.height() @@ -208,7 +180,7 @@ def paint_gl(): return if (__current_mode in [Mode.ADD, Mode.EDIT, Mode.DELETE] and - __current_points is None): + PointManager.point_set.empty()): return if __current_mode is Mode.ADD or __current_mode is Mode.DELETE: @@ -217,7 +189,7 @@ def paint_gl(): # the point set, which will be redrawn here. This action # is the same as adding a point since we just draw what is in # the point set. - draw_points(__current_points, Color.GREY) + draw_points(PointManager.point_set, Color.GREY) elif __current_mode is Mode.EDIT: raise NotImplementedError("Drawing for EDIT not implemented.") @@ -225,8 +197,8 @@ def paint_gl(): elif __current_mode is Mode.MOVE: # We have to repeatedly draw the points while we are showing the # move box. - if __current_points is not None: - draw_points(__current_points, Color.GREY) + if not PointManager.point_set.empty(): + draw_points(PointManager.point_set, Color.GREY) draw_selection_box(Color.BLACK) @@ -236,8 +208,7 @@ def paint_gl(): # Mark points that are selected in the bounding box # and draw them using the normal function highlight_selection() - draw_points(__current_points, Color.GREY) - + draw_points(PointManager.point_set, Color.GREY) def __clamp_x(x): """ @@ -296,7 +267,7 @@ def highlight_selection(): top_left = get_bb_top_left() bottom_right = get_bb_bottom_right() - for point in __current_points.points: + for point in PointManager.point_set.points: if box_hit(point.x, point.y, top_left[0], top_left[1], bottom_right[0], bottom_right[1]): @@ -358,7 +329,6 @@ def draw_selection_box(color): glEnd() - def clear_selection(): """ A helper designed to be called from the main window @@ -366,10 +336,8 @@ def clear_selection(): and mode files. This way you dont have to do something before the selection clears. """ - global __current_points - - if __current_points is not None: - __current_points.clear_selection() + if not PointManager.point_set.empty(): + PointManager.point_set.clear_selection() def draw_points(point_set, color): """ @@ -393,8 +361,7 @@ def draw_points(point_set, color): glViewport(0, 0, __WIDTH, __HEIGHT) - glPointSize(__current_points.point_size) - + glPointSize(PointManager.point_set.point_size) glBegin(GL_POINTS) for point in point_set.points: diff --git a/clusterview/point_list_widget.py b/clusterview/point_list_widget.py new file mode 100644 index 0000000..8ed3587 --- /dev/null +++ b/clusterview/point_list_widget.py @@ -0,0 +1,52 @@ +""" +Similar to the opengl_widget module, this module defines +helper functions for the point_list_widget. It is named +the same for convenience. The actual point_list_widget +is defined in the clusterview_ui.py file. +""" + +from .point_manager import PointManager + + +def __string_point_to_point(str_point): + """ + In the QListWidget points are stored as strings + because of the way Qt has list items defined. + + @param str_point The string of the form (x, y) to convert. + """ + + # 1. Split + elems = str_point.split(",") + + # 2. Take elements "(x" and "y)" and remove their first and + # last characters, respectively. Note that for y this + # function expects there to be a space after the comma. + x = elems[0][1:] + y = elems[1][1:-1] + + return (x, y) + +def item_click_handler(ctx, item): + """ + Handles an item becoming clicked in the list. + + This function is designed to be partially applied with the + main_window context in order to be able to trigger an opengl_widget + refresh. + + @param ctx The context. + @param item The clicked item. + """ + point = __string_point_to_point(item.text()) + + # TODO: Super slow linear search, should write a find_point function + # on the point_set in order to speed this up since PointSet + # is backed by a set anyway. + for p in PointManager.point_set.points: + if p.x == point[0] and p.y == point[1]: + p.select() + else: + p.unselect() + + ctx.opengl_widget.update() diff --git a/clusterview/point_manager.py b/clusterview/point_manager.py new file mode 100644 index 0000000..2d8c049 --- /dev/null +++ b/clusterview/point_manager.py @@ -0,0 +1,7 @@ +class PointManager(): + """ + A state class that represents the absolute state of the + world in regards to points. + """ + + point_set = None diff --git a/clusterview/points.py b/clusterview/points.py index b01fdac..be81f05 100644 --- a/clusterview/points.py +++ b/clusterview/points.py @@ -211,6 +211,9 @@ class PointSet: def viewport_width(self, width): self.__viewport_width = width + def empty(self): + return len(self.__points) == 0 + def clear_selection(self): """ Handy helper function to clear all selected points. diff --git a/main_window.py b/main_window.py index 9c670fb..6487651 100644 --- a/main_window.py +++ b/main_window.py @@ -12,6 +12,7 @@ from clusterview.mode_handlers import MODE_HANDLER_MAP, ogl_keypress_handler from clusterview.opengl_widget import (clear_selection, initialize_gl, paint_gl, resize_gl, set_drawing_mode, set_drawing_context) +from clusterview.point_list_widget import item_click_handler from clusterview_ui import Ui_MainWindow class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow): @@ -44,6 +45,11 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow): # into the function so that we can modify it as we please. self.opengl_widget.keyPressEvent = partial(ogl_keypress_handler, self) + # Same story here but this time with the itemClicked event + # so that when an element is clicked on in the point list it will + # highlight. + self.point_list_widget.itemClicked = partial(item_click_handler, self) + #----------------------------------------------- # OpenGL Graphics Handlers are set # here and defined in clusterview.opengl_widget. diff --git a/tests/test_point.py b/tests/test_point.py index 077c226..2b1fa9d 100644 --- a/tests/test_point.py +++ b/tests/test_point.py @@ -13,25 +13,25 @@ def test_move_point(): assert p.x == 5 and p.y == 5 def test_move_point_outside_screen_x_positive(): - p = Point(1, 2, 8, 100, 100) + p = Point(4, 4, 8, 100, 100) - with pytest.raises(ExceededWindowBoundsError): + with pytest.raises(ExceededWindowBoundsError) as exc_info: p.move(96, 0) def test_move_point_outside_screen_y_positive(): - p = Point(1, 2, 8, 100, 100) + p = Point(4, 4, 8, 100, 100) with pytest.raises(ExceededWindowBoundsError): p.move(0, 95) def test_move_point_outside_screen_x_negative(): - p = Point(1, 2, 8, 100, 100) + p = Point(4, 4, 8, 100, 100) with pytest.raises(ExceededWindowBoundsError): - p.move(-2, 0) + p.move(-5, 0) def test_move_point_outside_screen_y_negative(): - p = Point(1, 2, 8, 100, 100) + p = Point(4, 4, 8, 100, 100) with pytest.raises(ExceededWindowBoundsError): - p.move(0, -3) + p.move(0, -5) diff --git a/tests/test_point_set.py b/tests/test_point_set.py index 90aa2f8..8b545cd 100644 --- a/tests/test_point_set.py +++ b/tests/test_point_set.py @@ -3,6 +3,11 @@ import pytest from clusterview.points import Attribute, Point, PointSet +def test_empty(): + l = PointSet(3, 100, 100) + + assert l.empty() + def test_add_to_point_set(): l = PointSet(3, 100, 100) @@ -33,10 +38,10 @@ def test_remove_point_exact_click(): attribute = Attribute("thing", 1) l = PointSet(8, 100, 100) - l.add_point(1, 2, attrs=[attribute]) + l.add_point(4, 4, attrs=[attribute]) - p = Point(1, 2, 8, 100, 100) - l.remove_point(1, 2) + p = Point(4, 4, 8, 100, 100) + l.remove_point(4, 4) points = list(l.points) @@ -56,13 +61,13 @@ def test_remove_point_bounding_box(): attribute = Attribute("thing", 1) l = PointSet(8, 100, 100) - l.add_point(1, 2, attrs=[attribute]) + l.add_point(4, 4, attrs=[attribute]) - p = Point(1, 2, 8, 100, 100) + p = Point(4, 4, 8, 100, 100) # The click-point (2, 1) will be inside of our point size 8 # bounding box. - l.remove_point(2, 1) + l.remove_point(4, 4) points = list(l.points) @@ -78,12 +83,12 @@ def test_remove_point_bounding_box(): def test_attributes_must_be_array_of_attributes(): with pytest.raises(ValueError): l = PointSet(8, 100, 100) - l.add_point(1, 2, attrs=[1,2,3,4,5]) + l.add_point(4, 4, attrs=[1,2,3,4,5]) def test_clear_all_selected_points(): l = PointSet(8, 100, 100) - l.add_point(1, 2) - l.add_point(3, 4) + l.add_point(4, 4) + l.add_point(5, 5) for p in l.points: p.select()