feat: show loading progress #197

hunjixin · 2024-05-22T07:11:13Z

Resolves #133. change spinner to progressbar.

nazar-pc

This is a good start, thanks!
Left some suggestions on how to improve it further.

src/backend.rs

nazar-pc · 2024-05-23T20:31:51Z

src/backend.rs

+        match self {
+            LoadingStep::LoadingConfiguration => 0.0,
+            LoadingStep::ReadingConfiguration => 5.0,
+            LoadingStep::ConfigurationReadSuccessfully {
+                configuration_exists: _,
+            } => 10.0,
+            LoadingStep::CheckingConfiguration => 15.0,
+            LoadingStep::ConfigurationIsValid => 20.0,
+            LoadingStep::DecodingChainSpecification => 25.0,
+            LoadingStep::DecodedChainSpecificationSuccessfully => 30.0,
+            LoadingStep::CheckingNodePath => 35.0,
+            LoadingStep::CreatingNodePath => 40.0,
+            LoadingStep::NodePathReady => 45.0,
+            LoadingStep::PreparingNetworkingStack => 50.0,
+            LoadingStep::ReadingNetworkKeypair => 55.0,
+            LoadingStep::GeneratingNetworkKeypair => 60.0,
+            LoadingStep::WritingNetworkKeypair => 65.0,
+            LoadingStep::InstantiatingNetworkingStack => 70.0,
+            LoadingStep::NetworkingStackCreatedSuccessfully => 75.0,
+            LoadingStep::CreatingConsensusNode => 85.0,
+            LoadingStep::ConsensusNodeCreatedSuccessfully => 90.0,
+            LoadingStep::CreatingFarmer => 95.0,
+            LoadingStep::FarmerCreatedSuccessfully => 100.0,
+            LoadingStep::WipingFarm {
+                farm_index: _,
+                path: _,
+            } => 0.0,
+            LoadingStep::WipingFarmSuccessfully => 50.0,
+            LoadingStep::WipingNode { path: _ } => 80.0,
+            LoadingStep::WipingNodeSuccessfully => 100.0,


Naively you'd think this would work, but it is far from accurate. Different steps take different amount of time and thus progress with fixed 5% between steps is not accurate.

Also it doesn't account for the fact that there could be multiple steps in node creation and it definitely doesn't account for how many farms there are. If you have one farm and 20 it will take vastly different amount of time to complete that step. I see that you take that into consideration below, but it is still 5% for all farms in total.

This is why #133 says to do estimation first and scale everything accordingly to show realistic progress.

src/frontend/loading.rs

nazar-pc · 2024-05-23T20:36:40Z

src/frontend/loading.rs

+            gtk::ProgressBar {
+                #[watch]
+                set_fraction: model.progress,
+                set_show_text: true,
            },


I may have not been super clear, but the goal wasn't necessarily to remove the spinner.

While it is probably fine to do the way you did it, I was thinking about high-level steps shown with spinner and detailed steps below progress bar.

High level step would be "Creating farmer" and low-level would be creation and benchmarking of individual farms under that farmer.

Like title and subtitle if it makes sense.

at first i want to implement a spinner like this

GTK does not have native support.

and i am not think its a better idea to use title and subtitle. Implementing a complex solution may not provide much greater help. I think using a progress bar is more suitable, implementing a spinner with a similar effect as shown above could work as well. this will more friendly for both small farm and big farm.

for a more accurate for each farm, maybe need move LoadingStep type to backend, or add a notify in backend/farmer.rs file. but benchmarking need more deep change in farmer.

That is too complex for this purpose, not worth it.

I think simply adding title with main step above progress bar and making text below progress bar describing smaller inner steps will be sufficient.

For benchmarking we will indeed have to modify a farmer since it doesn't expose that information at the moment.

is this your meaning.

Yes, exactly!

simple set consensus node 20%, all farm occupy 60%, others occupy 20%. if need more accurate, maybe others nearly nothing, consensus node and all farm split 100% equaly (100%/(farmer_len+1).

Makes sense overall, though more farms does mean we will spend more time there, so if node fraction is decreased proportionally it'll make things more realistic, but I'm fine with it either way for initial version.

src/main.rs

nazar-pc

Thanks for making progress here and feel free to re-request review, there is a button on GitHub for this so I get a notification when you think PR is ready for review

nazar-pc · 2024-06-04T11:51:24Z

src/backend.rs

@@ -201,6 +226,7 @@ pub enum BackendNotification {
        #[allow(dead_code)]


This and above TODO should be removed by this PR

nazar-pc · 2024-06-04T11:52:14Z

src/frontend/loading.rs

                    }
                    LoadingStep::DecodedChainSpecificationSuccessfully => {
-                        "Decoded chain specification successfully".to_string()
+                        "Decoding chain specification".to_string()


This change needs to be reverted

nazar-pc · 2024-06-04T11:54:11Z

src/backend.rs

+        - LoadingStep::FarmerCreatedSuccessfully.percentage())
+        / (disk_farms.len() as f32);
+
+    let notifications = Arc::new(farmer::Notifications::default());


Please don't do this, this is not not notifications are supposed to work. There are already subscriptions for existing notifications, just follow that pattern instead of breaking abstractions.

nazar-pc · 2024-06-04T11:57:28Z

src/backend.rs

@@ -616,7 +642,8 @@ async fn load_configuration(
    notifications_sender
        .send(BackendNotification::Loading {
            step: LoadingStep::LoadingConfiguration,
-            progress: 0.0,
+            progress: LoadingStep::LoadingConfiguration.percentage(),
+            message: "loading configuration ...".to_string(),


I see all of these messages were copied from another file, but why did you remove capitalization everywhere?:

Suggested change

message: "loading configuration ...".to_string(),

message: "Loading configuration ...".to_string(),

nazar-pc · 2024-06-04T11:59:11Z

src/backend.rs

-    WipingNode {
-        path: PathBuf,
-    },
+    WipingFarm,


I don't think it is the right change to remove properties from this enum variant. In fact I think more variants should have more properties here!
Instead of sending message to the frontend, you should send LoadingStep such that there is enough information for frontend to render the message (kind of like it was before). Backend should not be responsible to rendering user-facing messages for frontend, there is a separation of responsibilities here.

step should contain all the information necessary. Moreover, since you have decided to add percentage as a field on this enum, progress field is not necessary either, step.progress() can be called by UI to render progress accordingly.

nazar-pc · 2024-06-04T12:00:52Z

src/main.rs

@@ -668,10 +668,18 @@ impl App {
    fn process_backend_notification(&mut self, notification: BackendNotification) {
        match notification {
            // TODO: Render progress


Again, once you implement rendering of progress, remove corresponding TODOs

nazar-pc reviewed May 23, 2024

View reviewed changes

hunjixin marked this pull request as draft May 24, 2024 03:20

hunjixin added 4 commits May 30, 2024 19:01

feat: show loading progress

383a6c4

feat: add wipe progress

29c7fbb

feat: update progress

c52eaa0

feat: rename basic_progress to percent

056d09d

hunjixin force-pushed the feat/load_progress branch from e16b2e5 to 056d09d Compare May 30, 2024 11:31

hunjixin added 3 commits May 30, 2024 19:33

remove empty line

f9e113f

feat: update message

103d5bd

feat: make title black and message grey

59a6440

hunjixin marked this pull request as ready for review May 30, 2024 12:54

chore: fix view format

2c16bf9

nazar-pc reviewed Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: show loading progress #197

feat: show loading progress #197

hunjixin commented May 22, 2024

nazar-pc left a comment

nazar-pc May 23, 2024

nazar-pc May 23, 2024

hunjixin May 24, 2024

nazar-pc May 24, 2024

hunjixin May 24, 2024

nazar-pc May 24, 2024

hunjixin May 30, 2024

nazar-pc May 30, 2024

nazar-pc left a comment

nazar-pc Jun 4, 2024

nazar-pc Jun 4, 2024

nazar-pc Jun 4, 2024

nazar-pc Jun 4, 2024

nazar-pc Jun 4, 2024

nazar-pc Jun 4, 2024

		@@ -201,6 +226,7 @@ pub enum BackendNotification {
		#[allow(dead_code)]

	message: "loading configuration ...".to_string(),
	message: "Loading configuration ...".to_string(),

feat: show loading progress #197

Are you sure you want to change the base?

feat: show loading progress #197

Conversation

hunjixin commented May 22, 2024

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment