Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactoring to avoid large modules #1761

Closed
totaam opened this issue Feb 2, 2018 · 6 comments
Closed

refactoring to avoid large modules #1761

totaam opened this issue Feb 2, 2018 · 6 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Feb 2, 2018

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.

@totaam
Copy link
Collaborator Author

totaam commented Feb 20, 2018

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)

@totaam
Copy link
Collaborator Author

totaam commented Feb 22, 2018

2018-02-22 17:34:37: antoine commented


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

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.

@totaam
Copy link
Collaborator Author

totaam commented Feb 23, 2018

2018-02-23 14:32:40: antoine commented


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

@totaam
Copy link
Collaborator Author

totaam commented Feb 24, 2018

2018-02-24 12:11:08: antoine commented


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

@totaam
Copy link
Collaborator Author

totaam commented Mar 1, 2018

2018-03-01 07:03:36: antoine commented


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

@totaam
Copy link
Collaborator Author

totaam commented Mar 1, 2018

This will do for this release.

Will follow up in #1778 and #1700.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant