xpra icon
Bug tracker and wiki

Opened 8 months ago

Closed 7 months ago

#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 (6)

comment:1 Changed 7 months 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 7 months ago by Antoine Martin (previous) (diff)

comment:2 Changed 7 months ago by Antoine Martin

Status: newassigned

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 7 months ago by Antoine Martin

Server base class split into mixins:

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 7 months ago by Antoine Martin (previous) (diff)

comment:4 Changed 7 months 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 7 months ago by Antoine Martin (previous) (diff)

comment:5 Changed 7 months 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

Minor fixups: r18620, r18609, r18606, r18600, r18599, r18597, r18590, r18585, r18581, r18580

Last edited 7 months ago by Antoine Martin (previous) (diff)

comment:6 Changed 7 months ago by Antoine Martin

Resolution: fixed
Status: assignedclosed

This will do for this release.

Will follow up in #1778 and #1700.

Note: See TracTickets for help on using tickets.