eduvpn / eduvpn-common

Code to be shared between eduVPN clients
MIT License
5 stars 3 forks source link

V2: Should CurrentServer be returned on GotConfig? #20

Closed jwijenbergh closed 10 months ago

jwijenbergh commented 1 year ago

Should the current server be sent as data to the GotConfig state?

Upside:

Downside:

rozmansi commented 1 year ago

This would look awkward then: the GotConfig callback data is not Config, but Server. Unless GotConfig state callback would provide data for Config, Server and ExpiryTimes together?

Initially, I was attempting to implement eduVPN client for Windows trying to use callback as much as possible to drive the GUI. However, the callbacks provide only essential and absolutely necessary data. But nobody is stopping you from calling CurrentServer() or ExpiryTimes() in the callback handler if you need this data. So, performance-wise the current solution seems best.

jwijenbergh commented 1 year ago

GotConfig state callback would provide data for Config, Server and ExpiryTimes together Possible indeed

It is possible that we should utilize callback data more. E.g. NoServer should maybe also contain the server list?

rozmansi commented 1 year ago

True. Callbacks are mandatory (AskLocation, OAuthStarted, AskProfile) not optional. If we decide to go this way, everything should be migrated to the callbacks making only those API functions that represent user-induced action remain.

Otherwise, we get some half-baked solution.

jwijenbergh commented 10 months ago

I think this is not a good improvement (title).

If we decide to go this way, everything should be migrated to the callbacks making only those API functions that represent user-induced action remain.

Maybe we could reconsider this later, but I think that only having the mandatory callbacks have data in them makes some sense