Code review comment for lp:~urbanape/ubuntu-sso-client/initial-darwin-port

Revision history for this message
Manuel de la Peña (mandel) wrote :

Great start for a succeful port, here there are a few comments:

1. There are several new headers added, we had to add an exception due to openssl to the license, can you update them (you can find the new header in trunk).
2. Lets remove alejandro from the headers :)
3. I think we need to refactor the following import so that it does not import from windows:

09 +if sys.platform in ('win32', 'darwin'):
10 from ubuntu_sso.keyring.windows import Keyring
11 else:
12 from ubuntu_sso.keyring.linux import Keyring

I good idea would be to rename the windows.py to something more generic, maybe pykeyring?

4. Same as the above with:

1150 +if sys.platform in ('win32', 'darwin'):
1151 from ubuntu_sso.qt.main import windows
1152 source = windows

5. There is some code duplication here:

1022 +# Internal NetworkManager State constants
1023 +NM_STATE_UNKNOWN = 0
1024 +NM_STATE_UNKNOWN_LIST = [NM_STATE_UNKNOWN]
1025 +NM_STATE_ASLEEP_OLD = 1
1026 +NM_STATE_ASLEEP = 10
1027 +NM_STATE_ASLEEP_LIST = [NM_STATE_ASLEEP_OLD,
1028 + NM_STATE_ASLEEP]
1029 +NM_STATE_CONNECTING_OLD = 2
1030 +NM_STATE_CONNECTING = 40
1031 +NM_STATE_CONNECTING_LIST = [NM_STATE_CONNECTING_OLD,
1032 + NM_STATE_CONNECTING]
1033 +NM_STATE_CONNECTED_OLD = 3
1034 +NM_STATE_CONNECTED_LOCAL = 50
1035 +NM_STATE_CONNECTED_SITE = 60
1036 +NM_STATE_CONNECTED_GLOBAL = 70
1037 +# Specifically don't include local and site, as they won't let us get to server
1038 +NM_STATE_CONNECTED_LIST = [NM_STATE_CONNECTED_OLD,
1039 + NM_STATE_CONNECTED_GLOBAL]
1040 +NM_STATE_DISCONNECTED_OLD = 4
1041 +NM_STATE_DISCONNECTED = 20
1042 +# For us, local and site connections are the same as diconnected
1043 +NM_STATE_DISCONNECTED_LIST = [NM_STATE_DISCONNECTED_OLD,
1044 + NM_STATE_DISCONNECTED,
1045 + NM_STATE_CONNECTED_LOCAL,
1046 + NM_STATE_CONNECTED_SITE]

I think we can add that in a single module to be shared. I'll be adding some bugs following this brach so that we can track our progress.

review: Needs Fixing

« Back to merge proposal