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

Connection monitor changes #282

Merged
merged 33 commits into from
Jul 31, 2021
Merged

Connection monitor changes #282

merged 33 commits into from
Jul 31, 2021

Conversation

hussainmohd-a
Copy link
Collaborator

Setunderlying network logic of v053e is modified.

Now new settings introduced, Set all available networks to the
VPN. If the setting is enabled, the underlying network will be
set with all the available networks. With active network
as first in the list.

If the settings is disabled, only active network will be set in the
underlying network option.

Single threaded handler is implemented to process the messages
from the connectivitiy-manager.

Setunderlying network logic of v053e is modified.

Now new settings introduced, Set all available networks to the
VPN. If the setting is enabled, the underlying network will be
set with all the available networks. With active network
as first in the list.

If the settings is disabled, only active network will be set in the
underlying network option.

Single threaded handler is implemented to process the messages
from the connectivitiy-manager.
@hussainmohd-a
Copy link
Collaborator Author

Already available ConnectionCapabilityMonitor file has been replaced with ConnectionMonitor file.

@hussainmohd-a hussainmohd-a requested a review from ignoramous May 5, 2021 21:00
Copy link
Collaborator

@ignoramous ignoramous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rework is required. Please address comments.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/activity_settings_screen.xml Outdated Show resolved Hide resolved
Rename of companion objects - MSG_ADD_ACTIVE_NETWORK,
MSG_ADD_ALL_NETWORKS.

Removed repeated comments in ConnectionMonitor.

Removal of super methods for those where the super method
has empty implementation.

Cancel all the messages before sending the message from
onCapabilitiesChanged and onLinkPropertiesChanged

Capture the active network inside handler.

Changes on processActiveNetwork() and processAllNetwork()
Copy link
Collaborator

@ignoramous ignoramous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reply to all review comments when done!

Removed the onUpdateActiveNetwork from the Network Listener.

Removed unused constructMessage(message: Message?, what: Int).

Made the processMessage into inline.

Added logs in processActiveNetwork and processAllNetworks
method.

Modified the hasInternet and isVPN variables to method.
Added logs for onNetworkDisconnected().
Introduced new settings for the users to select what action button
to show in the notification bar.
As of now three options provided.
1. No action button.
2. STOP action button.
3. RethinkDNS modes (DNS & DNS+Firewall mode)

1 - No action button will be available in notification bar.
2 - Like earlier, the STOP button will be shown.
3 - DNS and DNS+Firewall mode change - user can change the
mode from the action button.
Copy link
Collaborator

@ignoramous ignoramous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please improve logging, fix the data-race bug outlined in ConnectionMonitor, and clarify the use of CountDownTimers, which look woefully inefficient at first look, among other changes.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
Removed usage of Global variable from the Notification receiver.

Removed commented out code from VPN service class.
Modifications of log statements.

ForceUpdate - if onUserPreferenceChanged is changes, the
messages should be processed forcefully regardless of the
current and new network.

Modified and changes on logs and string literals.
app/src/main/java/com/celzero/bravedns/util/OrbotHelper.kt Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
Instead of raw integers, the constants are used.

Changes in logs.

Changes in strings literals.
Copy link
Collaborator

@ignoramous ignoramous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • please use Koin where-ever applicable.
  • please make the changes to ConnectionMonitor to get rid of getNetworkList method and currentActiveNetworks variable, and add a new onVpnStarted method.
  • add accessor methods to persistent state values that need it, and make sure to hydrate the initial state variables as necessary.
  • seek to simplify code and reduce clutter.

app/src/main/java/com/celzero/bravedns/util/Constants.kt Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/java/com/celzero/bravedns/util/Constants.kt Outdated Show resolved Hide resolved
ConnectionMonitor changes - Removed the method
getNetworkList() and introduced new method onVpnStarted().
Moved the currentActiveNetworks to NetworkRequestHandler.

Removed the global variable.
Rename of constant variables.
Change in log statements.
Code formatting.
@ignoramous
Copy link
Collaborator

#277

@hussainmohd-a hussainmohd-a requested a review from ignoramous May 23, 2021 15:28
Kotlin style formatting on some of the files.
Removed some of the global variables -
isBackgroundEnabled.

Modified some of the logics in if-else statements where
the else statements had just logs.

Remove some raw int's.

Notification in case of failure loading the
firewall rules.

Notification action button color now depends on
system theme.

Changes in check for the background accessibility.

Changes in the logger statements.

Changes in the comments.

Changes in logic of handling network connections.

Adding the network request count every one minute
to shared-pref.
Update new-line in ConnectionMonitor.
Copy link
Collaborator Author

@hussainmohd-a hussainmohd-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed some of the global variables -isBackgroundEnabled.
Modified some of the logics in if-else statements where the else statements had just logs.
Remove some raw int's.
Notification in case of failure loading the firewall rules.
Notification action button color now depends on system theme.
Changes in check for the background accessibility.
Changes in the logger statements.
Changes in the comments.
Changes in logic of handling network connections.
Adding the network request count every one minute to shared-pref.

Port 853 reserved port name change from "domain-s"
to "secure-dns".
According to the documentation, the establish function
will return null if the VpnService has not been prepared,
however we are using prepare() prior to establish().

Some cases, the ParcelFileDescriptor is null. This results
the app in the red waiting state during the vpn
restart. Fix will stop the vpn complete in case if the
builder.establish() returned as null.

Move the code - open the vpn profile to a common
place. (called from both Home screen and about
page).
Copy link
Collaborator

@ignoramous ignoramous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic comments, mostly.

Changes / addition of new comments.
Rename of methods and variables.
Code-abstract.
Code-format.
New check for active notification for load failure.
Code formatting.
Issue fix for failure to load image for the applications and
category in Firewall app fragment.
Code refractor.
Changes in log statements.
Changes in error handling.
Removed unwanted code comments.
Changes in code comments.
Changes in variable or method names.
Notification now launches Firewall activity instead of
default launcher activity.

Removed unused code.
Code refractor.
Changes in log statements.
Removed unwanted code comments.
Changes in code comments.
Changes in variable or method names.
Copy link
Collaborator

@ignoramous ignoramous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost home.

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

Successfully merging this pull request may close these issues.

3 participants