fix(ui): make internet resource off by default by conectado · Pull Request #6518 · firezone/firezone

conectado · 2024-08-31T05:07:01Z

With this PR we made internet resource disabled by default.

Since no other resource is disalable and internet resource behavior is particular we remove all client code to make non internet resource disalable.

Also, since the portal never makes the internet resource that can't be disabled we remove the whole code path to handle that.

Additionally, some other smaller refactors across the UI wrt internet resource

Fix #6509

vercel · 2024-08-31T05:07:05Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
firezone	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 4, 2024 4:56pm

github-actions · 2024-09-02T19:15:05Z

Bencher

Report	Wed, September 4, 2024 at 17:05:40 UTC
Project	Firezone
Branch	fix/internet-resource-ui
Testbed	github-actions

Click to view all benchmark results

Benchmark	Throughput	Throughput Results bits/s \| (Δ%)	Throughput Lower Boundary bits/s \| (%)
direct-tcp-client2server	✅ (view plot)	249,081,454.61 (+1.02%)	238,104,360.87 (95.59%)
direct-tcp-server2client	✅ (view plot)	255,825,452.08 (+1.80%)	243,473,768.23 (95.17%)
direct-udp-client2server	✅ (view plot)	307,110,372.58 (+5.78%)	271,090,526.79 (88.27%)
direct-udp-server2client	✅ (view plot)	398,134,354.69 (-0.90%)	388,364,859.09 (97.55%)
relayed-tcp-client2server	✅ (view plot)	248,353,066.40 (-0.57%)	240,321,338.22 (96.77%)
relayed-tcp-server2client	✅ (view plot)	262,640,789.51 (+1.18%)	249,878,775.24 (95.14%)
relayed-udp-client2server	✅ (view plot)	227,793,889.73 (-1.84%)	220,878,630.63 (96.96%)
relayed-udp-server2client	✅ (view plot)	362,181,036.15 (+7.27%)	316,663,607.09 (87.43%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

thomaseizinger

Overall LGTM

kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/ViewResource.kt

thomaseizinger · 2024-09-03T20:37:23Z

kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/ViewResource.kt

+    }
+}
+
+fun Resource.toViewResource(resourceState: ResourceState): ViewResource {


I think a ctor on ViewResource would be more natural.

I like to do the conversion as resource.toResourceViewModel instead of ResourceViewModel.from(...) and pass the resource in.

Sure. My reasoning would be that if you think in terms of dependencies, then the view-stuff is okay to know about the actual entities but the entities shouldn't know about the view. Like, if you were to extract the view into a separate module / crate then you can only do that if the conversion function lives on the view-model type (or completely independent of both).

thomaseizinger · 2024-09-03T20:38:38Z

kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/ViewResource.kt

+fun internetResourceDisplayName(resource: Resource, state: ResourceState): String {
+    return if (!resource.canBeDisabled) {
+        "$OnSymbol ${resource.name}"
+    } else {
+        "${state.stateSymbol()} ${resource.name}"
+    }
+}
+
+fun displayName(resource: Resource, state: ResourceState): String {
+    if (resource.isInternetResource()) {
+        return internetResourceDisplayName(resource, state)
+    } else {
+        return resource.name
+    }
+}


I'd expect utilities to be below the function that uses them.

thomaseizinger · 2024-09-03T20:40:35Z

rust/gui-client/src-tauri/src/client/gui.rs

+        let internet_resource = self
+            .status
+            .internet_resource()
+            .ok_or(anyhow!("Tunnel not ready"))?;


Suggested change

.ok_or(anyhow!("Tunnel not ready"))?;

.context("Tunnel not ready")?;

rust/gui-client/src-tauri/src/client/gui/system_tray.rs

rust/gui-client/src-tauri/src/client/gui/system_tray/builder.rs

rust/gui-client/src-tauri/src/client/settings.rs

swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

jamilbk · 2024-09-04T05:59:03Z

kotlin/android/app/src/main/java/dev/firezone/android/core/data/Repository.kt

+    return if (this.isEnabled()) {
+        ResourceState.DISABLED
+    } else {
+        ResourceState.ENABLED
+    }


Are these reversed? Seems a tad confusing.

this function "toggles" it so it swap the state out

jamilbk · 2024-09-04T06:04:42Z

kotlin/android/app/src/main/java/dev/firezone/android/core/data/Repository.kt

+public const val ON_SYMBOL: String = "<->"
+public const val OFF_SYMBOL: String = " — "


Should these be defined on the companion object at the bottom?

Also I think public might be redundant?

Should these be defined on the companion object at the bottom?

Is there any benefit to that? Also, shouldn't in that case be a companion to ResourceState?

jamilbk · 2024-09-04T06:10:14Z

kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/ResourceViewModel.kt

+    return "${state.stateSymbol()} ${resource.name}"
+}
+
+fun Resource.toResourceViewModel(resourceState: ResourceState): ResourceViewModel {


Shouldn't this function be defined on the Resource class?

It's defined in the Resource class but not on the Resource module.

I converted this to a constructor on ResourceViewModel

jamilbk · 2024-09-04T06:13:49Z

kotlin/android/app/src/main/java/dev/firezone/android/features/session/ui/SessionActivity.kt

+        tunnelService?.internetResourceToggled(internetState().toggle())
+        refreshList()
+        Log.d(TAG, "Internet resource toggled ${internetState()}")


Next two lines only make sense to execute if the tunnelService is valid, no?

Suggested change

tunnelService?.internetResourceToggled(internetState().toggle())

refreshList()

Log.d(TAG, "Internet resource toggled ${internetState()}")

tunnelService?.let {

it.internetResourceToggled(internetState().toggle())

refreshList()

Log.d(TAG, "Internet resource toggled ${internetState()}")

}

is there any point at which this can actually happen to be nil? Maybe I just use !!?

vercel bot deployed to Preview August 31, 2024 05:07 View deployment

vercel bot deployed to Preview September 2, 2024 19:08 View deployment

vercel bot deployed to Preview September 2, 2024 20:19 View deployment

conectado force-pushed the fix/internet-resource-ui branch from 7da9e1d to 3f05ea8 Compare September 3, 2024 06:48

vercel bot deployed to Preview September 3, 2024 06:50 View deployment

vercel bot deployed to Preview September 3, 2024 18:58 View deployment

vercel bot deployed to Preview September 3, 2024 19:33 View deployment

vercel bot deployed to Preview September 3, 2024 20:08 View deployment

conectado requested a review from thomaseizinger September 3, 2024 20:17

vercel bot deployed to Preview September 3, 2024 20:42 View deployment

vercel bot deployed to Preview September 3, 2024 20:45 View deployment

thomaseizinger approved these changes Sep 3, 2024

View reviewed changes

vercel bot deployed to Preview September 3, 2024 21:40 View deployment

vercel bot deployed to Preview September 3, 2024 22:21 View deployment

conectado force-pushed the fix/internet-resource-ui branch from 8d65387 to e0f4735 Compare September 3, 2024 22:31

vercel bot deployed to Preview September 3, 2024 22:35 View deployment

vercel bot deployed to Preview September 3, 2024 23:37 View deployment

vercel bot deployed to Preview September 3, 2024 23:59 View deployment

vercel bot deployed to Preview September 4, 2024 00:45 View deployment

vercel bot deployed to Preview September 4, 2024 01:06 View deployment

conectado changed the title ~~Fix/internet resource UI~~ fix(ui): make internet resource off by default Sep 4, 2024

conectado marked this pull request as ready for review September 4, 2024 05:43

conectado and others added 6 commits September 3, 2024 22:44

wip

b2902ed

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

disable as default for tauri

e5d6250

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

change name according to enable/disable internet resource

7a5aff2

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

ui for macos

64cf358

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

ui for ios

1fb9d9d

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

fix android ui

68d710d

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

conectado and others added 11 commits September 3, 2024 22:44

fix status symbols

a8a6b53

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

fix swift

16d0d0e

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

fix linting and rename viewresource

da8cf66

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

fix more android linting

a6ca74a

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

remove can be disabled and assume true

1afe6c9

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

apply pr feedback

0773983

Signed-off-by: conectado <gabrielalejandro7@gmail.com>

fix tauri tests

2ad1c80

fix tauri test

9fcd612

remove trailing whitespace

6594d9b

remove trailing white spaces

5083051

Remove trailing whitespace

bd9c398

conectado force-pushed the fix/internet-resource-ui branch from 5376c25 to bd9c398 Compare September 4, 2024 05:45

conectado requested review from ReactorScram and jamilbk September 4, 2024 05:45

vercel bot deployed to Preview September 4, 2024 05:47 View deployment

jamilbk reviewed Sep 4, 2024

View reviewed changes

apply pr feedback

a0a65e3

vercel bot deployed to Preview September 4, 2024 16:28 View deployment

remove dependency from resource to view resource

ea3aeae

vercel bot deployed to Preview September 4, 2024 16:42 View deployment

fix kotlin linting

e6f8c4d

vercel bot deployed to Preview September 4, 2024 16:56 View deployment

conectado added this pull request to the merge queue Sep 4, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 4, 2024

conectado added this pull request to the merge queue Sep 4, 2024

Merged via the queue into main with commit 700b056 Sep 4, 2024

conectado deleted the fix/internet-resource-ui branch September 4, 2024 19:31

	.ok_or(anyhow!("Tunnel not ready"))?;
	.context("Tunnel not ready")?;

		public const val ON_SYMBOL: String = "<->"
		public const val OFF_SYMBOL: String = " — "

Conversation

conectado commented Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bencher

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

conectado commented Aug 31, 2024 •

edited

Loading

vercel bot commented Aug 31, 2024 •

edited

Loading

github-actions bot commented Sep 2, 2024 •

edited

Loading