Refactoring and testing of Interactor classes #1570
Conversation
swsrkty
changed the title
mslib/msui/mpl_pathinteractor.py
Outdated
| self.ax.draw_artist(t) | ||
|
|
||
|
|
||
| class PathL_GCPlotter(PathPlotter): |
| waypoints_list.append((lat, lon, flightlevel, location, comments)) | ||
| return waypoints_list | ||
| waypoints_list1.append((lat, lon, flightlevel, location, comments)) | ||
| waypoints_list2.append(ft.Waypoint(lat, lon, flightlevel, location, comments)) |
There was a problem hiding this comment.
As most of the mslib stuff uses the list of ft.Waypoint's, I would try to remove the need of the waypoints_list1.
| @@ -87,84 +93,55 @@ def __init__(self): | |||
| self.params["basemap"].update(self.config["predefined_map_sections"][section]["map"]) | |||
| print(self.params) | |||
| self.bbox_units = self.params["bbox"] | |||
| self.wps = load_from_ftml(filename) | |||
| self.wpslist = [] | |||
| self.fig.clear() | ||
| self.ax = self.fig.add_subplot(111, zorder=99) | ||
| self.path = [(wp[0], wp[1], datetime.datetime.now()) for wp in self.wps] | ||
| self.vertices = [list(a) for a in (zip(self.wp_lons, self.wp_lats))] |
There was a problem hiding this comment.
This should be removed. The "vertices" of the msui classes are often in different units then lon/lat. The restructuring of using update_from_waypoints should have made this superfluous anyway.
|
|
||
| # plot path | ||
| def TopViewPath(self): |
There was a problem hiding this comment.
The function names do not follow the PEP8 convention. Also adding TopView here seems redundant.
I think it is sensible to have small functions plotting the individual plot components. However calling three functions without any argument in main() seems like a strange design. Why not only one? In principle you could put everything in the constructor - or make a simple function doing all that.
What makes sense is reusing the MyFigure class for all plots of one type. But maybe the draw() function should not contain the loop. Instead the loop should IMO go into the main() function and call a draw function with flightpath etc. as arguments.
I also suspect that this does not work properly for multiple flights as, e.g., the label functions must be called after changing the flight.
But maybe I am mistaken, I haven't tested it in practice.
autoplot.py
Outdated
| @@ -0,0 +1,58 @@ | |||
| import click | |||
There was a problem hiding this comment.
see template.py, please add the common header. Add your name as initial author
There was a problem hiding this comment.
the module should go to mslib.utils
autoplot.py
Outdated
| for flight, section, vertical, filename, init_time, time in \ | ||
| config["automated_plotting_flights"]: | ||
| for url, layer, style, elevation in config["automated_plotting_hsecs"]: | ||
| if(intv == 0): |
There was a problem hiding this comment.
the brackets are unusual, not needed.
brackets are used for multiline statements and they have a blank between if and (
There was a problem hiding this comment.
if is a condition, not a function.
There was a problem hiding this comment.
https://peps.python.org/pep-0008/ search for if-statement
autoplot.py
Outdated
| i = 1 | ||
| time = starttime | ||
| while(time <= endtime): | ||
| print(time) |
There was a problem hiding this comment.
may be replace print by logging.info, logging.debug
| @@ -69,6 +69,27 @@ | |||
| except IOError: | |||
| logging.error(f'"{MSUI_SETTINGS}" can''t be created') | |||
|
|
|||
| MSS_AUTOPLOT = os.getenv('MSS_AUTOPLOT', os.path.join(MSUI_CONFIG_PATH, "mss_autoplot.json")) | |||
There was a problem hiding this comment.
add a ToDo, # ToDo refactor to a function
| else: | ||
| inittime = itime | ||
| time = datetime.strptime(vtime, "%Y-%m-%dT" "%H:%M:%S") | ||
| if(ftrack != ""): |
There was a problem hiding this comment.
this "(" is only used for multiline statements and then with a blank after the "if"
see if-statement in
https://peps.python.org/pep-0008/
|
This was part of another pull request already. |
Refactoring Interactor module to separate Interactor classes from Plotting classes.