Context Navigation

← Previous Ticket
Next Ticket →

#1761 closed task (fixed)

refactoring to avoid large modules

Reported by:	Antoine Martin	Owned by:	Antoine Martin
Priority:	major	Milestone:	2.3
Component:	core	Version:	2.2.x
Keywords:		Cc:

Description

As of r18260, we have some modules that are just too big.

$ find xpra/ -type f -name "*.py" -exec bash -c 'cat {}|wc -l|xargs echo -n;echo " {}"' \;  | sort -n | tail -n 40
589 xpra/client/window_backing_base.py
602 xpra/x11/desktop_server.py
678 xpra/platform/win32/lsa_logon_lib.py
714 xpra/x11/gtk2/models/base.py
720 xpra/x11/gtk2/models/core.py
733 xpra/client/client_window_base.py
733 xpra/x11/xkbhelper.py
736 xpra/os_util.py
747 xpra/platform/win32/wndproc_events.py
770 xpra/x11/gtk2/models/window.py
773 xpra/net/file_transfer.py
798 xpra/util.py
834 xpra/platform/darwin/gui.py
890 xpra/x11/x11_server_core.py
895 xpra/sound/gstreamer_util.py
919 xpra/clipboard/clipboard_base.py
946 xpra/server/proxy/proxy_instance_process.py
950 xpra/platform/xposix/gui.py
1014 xpra/client/gtk_base/client_launcher.py
1031 xpra/client/client_base.py
1068 xpra/scripts/server.py
1100 xpra/client/gl/gl_window_backing_base.py
1100 xpra/net/protocol.py
1103 xpra/gtk_common/gtk_util.py
1173 xpra/client/gtk_base/session_info.py
1208 xpra/platform/win32/gui.py
1241 xpra/x11/server.py
1307 xpra/client/gtk_base/gtk_client_base.py
1379 xpra/scripts/config.py
1474 xpra/client/gtk_base/gtk_tray_menu_base.py
1528 xpra/client/gtk_base/gtk_client_window_base.py
1780 xpra/server/server_core.py
1934 xpra/server/window/window_video_source.py
2088 xpra/net/mdns/pybonjour.py
2276 xpra/server/window/window_source.py
2632 xpra/server/source.py
3402 xpra/scripts/main.py
3794 xpra/server/server_base.py
3904 xpra/client/ui_client_base.py
4717 xpra/platform/win32/constants.py

Some of those are fine: constants or external modules (ie: pybonjour), others deserve to be refactored.

This ticket is also in preparation for #1700.

Change History (7)

comment:1 Changed 4 years ago by Antoine Martin

First round:

r18499: server base: split control commands
r18498: main: split command line parsing
r18495 + r18497: server base: logical groups of methods
r18496: server core: logical groups of methods

ui client:

r18492: split rpc bits
r18491: split audio bits
r18489: split webcam bits
r18503: split window metadata and window filters to their own module
r18504 + r18505: better grouping of methods
r18508: split hello and caps parsing to their respective method groups (r18511 for server mmap prefixing)
r18510: split packet handler initialization to method groups

Result:

xpra/scripts/main.py: 2229
xpra/server/server_base.py: 3341 lines
xpra/client/ui_client_base.py: 3009 lines

Still big, but more readable and manageable.

Some of those changes can also help us write better unit tests.(#847)

Last edited 4 years ago by Antoine Martin (previous) (diff)

comment:2 Changed 4 years ago by Antoine Martin

Status:	new → assigned

Lots more gradual changes to ui client, eventually splitting the code into mixins:

r18512: clipboard mixin
r18514: notification mixin
r18516: window mixin
r18517: move input devices into window mixin
r18519: move systray forwarding into window mixin
r18521: mmap mixin
r18522: remote logging mixin
r18525: display / scaling / desktop mixin
r18526: move all mixins to a new "mixins" submodule
r18529: network: ping / info request mixin
r18530: encodings mixin
r18531: add stub mixin base class
r18532: xpra tray mixin
r18534: unit tests for mmap and network-state mixins
r18535: unit tests for remote logging mixin

Minor fixups and tweaks: r18513, r18515, r18518, r18520, r18523, r18524, r18527, r18527, r18528, r18533, r18533

Not yet dealt with:

init_ui is still a bit messy
more consistent namespace for mixin variables, try to distinguish: settings (command line, env vars), server settings (from hello or update packets), state (focus, buttons, etc)
some timers can now be tracked and cancelled more easily (ie: ping timers)

The ui client is now much smaller (~620 lines) and all the mixins file sizes are now much more reasonable:

$ wc -l xpra/client/ui_client_base.py xpra/client/mixins/*
   620 xpra/client/ui_client_base.py
   562 xpra/client/mixins/audio_client.py
   180 xpra/client/mixins/clipboard_client.py
   517 xpra/client/mixins/display_client.py
   293 xpra/client/mixins/encodings.py
     4 xpra/client/mixins/__init__.py
   123 xpra/client/mixins/mmap_client.py
   189 xpra/client/mixins/network_state.py
   155 xpra/client/mixins/notification_client.py
    94 xpra/client/mixins/remote_logging.py
   119 xpra/client/mixins/rpc_client.py
    36 xpra/client/mixins/stub_client_mixin.py
   125 xpra/client/mixins/tray_client.py
   310 xpra/client/mixins/webcam_forwarder.py
  1296 xpra/client/mixins/window_client.py
  4623 total

The line count has actually increased, but the code is in much better shape, much more readable and maintainable.

Next: the server.

comment:3 Changed 4 years ago by Antoine Martin

Server base class split into mixins:

r18539: notifications
r18540: webcam
r18541: clipboard
r18543: audio
r18544: file-transfers + printing
r18545: mmap
r18546: input devices
r18548: start commands
r18549: dbus rpc
r18550: encodings
r18551: remote logging
r18554: network state
r18555: display management
r18556: window management

Minor fixups in r18542, r18547, r18552, r18553, r18557

Still TODO for server base class:

do_cleanup vs cleanup: make it consistent
setup should not need opts
parse_hello_ui_clipboard and more pollution in ui hello parsing
clipboard is broken? (client side problem?)
client_reset in clipboard helper
init_sockets printing

Which gives us:

$ wc -l xpra/server/server_base.py xpra/server/mixins/*
   973 xpra/server/server_base.py
   297 xpra/server/mixins/audio_server.py
   283 xpra/server/mixins/child_command_server.py
   189 xpra/server/mixins/clipboard_server.py
   102 xpra/server/mixins/dbusrpc_server.py
   251 xpra/server/mixins/display_manager.py
   212 xpra/server/mixins/encoding_server.py
   266 xpra/server/mixins/fileprint_server.py
     4 xpra/server/mixins/__init__.py
   401 xpra/server/mixins/input_server.py
    53 xpra/server/mixins/logging_server.py
    40 xpra/server/mixins/mmap_server.py
   156 xpra/server/mixins/networkstate_server.py
   143 xpra/server/mixins/notification_forwarder.py
   543 xpra/server/mixins/server_base_controlcommands.py
    31 xpra/server/mixins/stub_server_mixin.py
   247 xpra/server/mixins/webcam_server.py
   305 xpra/server/mixins/window_server.py
  4496 total

We should write a lot more unit tests for those mixins: #1773

Last edited 4 years ago by Antoine Martin (previous) (diff)

comment:4 Changed 4 years ago by Antoine Martin

Now done for "Server Source",

r18560: audio mixin and ServerSource renamed to ClientConnection
r18561: mmap mixin
r18562: clipboard mixin
r18563: file-transfers + printing
r18564: network-state (pings)
r18565: client info (build info, version, proxy details, etc)
r18566 + r18567: rename mixins
r18570: notifications refactoring
r18571: dbus mixin
r18572: window management
r18573: encodings
r18574: idle detection
r18575: input handling (keyboard, pointer)
r18576: av-sync
r18577: client-display state
r18569: expose the list of packet handlers via xpra info

Minor fixups in r18568.

The result:

$ wc -l xpra/server/source/*
   479 xpra/server/source/audio_mixin.py
    61 xpra/server/source/avsync_mixin.py
   449 xpra/server/source/client_connection.py
    88 xpra/server/source/clientdisplay_mixin.py
   127 xpra/server/source/clientinfo_mixin.py
   105 xpra/server/source/clipboard_connection.py
    36 xpra/server/source/dbus_mixin.py
   540 xpra/server/source/encodings_mixin.py
   146 xpra/server/source/fileprint_mixin.py
    93 xpra/server/source/idle_mixin.py
     4 xpra/server/source/__init__.py
   101 xpra/server/source/input_mixin.py
   114 xpra/server/source/mmap_connection.py
   105 xpra/server/source/networkstate_mixin.py
   235 xpra/server/source/source_stats.py
    28 xpra/server/source/stub_source_mixin.py
   592 xpra/server/source/windows_mixin.py
  3303 total

New TODO:

unit tests
move webcam server code there, webcam forwarding should be per-client (and think about handling more than one per client?)
mmap still referenced from other mixins: encodings and windows
suspend + resume code should also suspend audio?
networkstate: pings require statistics object
don't register packet handlers if feature is off
minimize import deps on mixin load (ie: done for window source in r18578)
modifier_client_keycodes still ugly

Last edited 4 years ago by Antoine Martin (previous) (diff)

comment:5 Changed 4 years ago by Antoine Martin

Lots more updates:

r18656 disable more features and their packet handlers if we find that we cannot load them or that they are disabled by the configuration
r18654 import as needed
r18653 docstrings
r18652 generalize init_sockets so we can move some domain specific checks to the fileprint mixin
r18651 generalize protocol cleanup callbacks, move clipboard cleanup to this callback
r18650 move system-tray to x11 server, don't pass options to setup method
r18624 move server info bits to a mixin, move remaining encoding attributes to the encodings mixin
r18623 move file+print to a mixin, only init aliases after we have initialized the packet handlers
r18598 gtk cleanup, use MRO, fix clipboard, consistency, etc
r18596 generically call parse_client_caps on all superclasses
r18595 try harder to split state variables from configuration options
r18594 add av-sync unit test, minor cleanups
r18593 webcam: tests, use libyuv, etc
r18592 move idle handling code to client-connection, generalize init_state source mixin method
r18591 rename server mixin test, add source mixin test
r18588 move webcam handling to a source mixin, so we can have one or more virtual webcam per connection
r18584 split caps into smaller methods, use namespace and only flatten the dicts as late as possible
r18583 move "scaling-control" option to encodings where it belongs, expose more info from display manager
r18579 move window-icon code to a mixin
r18578 reduce imports at module load time: import window source when needed instead
r18577 move client-display code to a mixin