Add sending regular progress messages #16

tsh-xx · 2017-06-02T13:06:33Z

Add interval setting to configuration screen
Incremented revision

Resolves #15

I was unsure about using the devel branch or master - seems they've diverged. I started working with master, and currently haven't managed to demonstrate devel working (without my changes).

Coding style isn't great, it was more of a challenge than I expected to get this to work - so given some pointers I can make updates if necessary. At least anyone who needs it can try my version and give feedback.

Default interval is 30 min (can be changes in the configuration page) First and last messages should be suppressed (based on estimated print time) Incremented revision

prahjister · 2017-06-02T13:11:02Z

I'm sorry I'm a git noob. How can I test this?

foosel · 2017-06-02T13:22:12Z

Awesome! 😄

I'll take a proper look ASAP - hopefully early next week, if no unforeseen OctoPrint emergencies get in the way!

foosel

There are sadly some huge TODOs left. I have to admit I didn't understand at all what is going on in the progress handler, there's so much back and forth of variables and calculations with seemingly arbitrary values that it's unclear what is happening for what reason and when. All in all it looks to be way too complicated for what it is supposed to achieve, so I think some clarification is required. There are also some typos in there that would cause stuff not to work as expected at all, and I think a parameter you added to the connection handler probably made connection to pushbullet stop entirely.

foosel · 2017-06-07T14:24:46Z

octoprint_octobullet/__init__.py


 	def _connect_bullet(self, apikey, channel_name=""):
 		self._bullet, self._sender = self._create_sender(apikey, channel_name)

+	#~~ progress message helpers
+	def _sanitize_current_data(self, currentData):


I think it's dangerous to just use arbitrary values (1000, 0, ...) instead of the actual values. If they are None they are None for a reason - they are unknown or not stable and shouldn't be displayed to the user.

I borrowed this from the detailed progress plugin, I've simplified it now.

foosel · 2017-06-07T14:24:52Z

octoprint_octobullet/__init__.py

+		if (currentData["progress"]["printTimeLeft"] == None):
+			currentData["progress"]["printTimeLeft"] = currentData["job"]["estimatedPrintTime"]
+		if (currentData["progress"]["printTimeLeft"] == None):
+			self._logger.info("Messasge: Still got no print time {}".format(currentData["progress"]["printTimeLeft"]))


This and most of the other info logging lines should be self._logger.debug and you also had a consistent typo there: "Messasge" instead of "Message" (but why the prefix at all, that it's a log message is fairly visible if it lands in the log anyhow, no need to explicitly state this?).

foosel · 2017-06-07T14:24:54Z

octoprint_octobullet/__init__.py

+
+		return currentData
+
+	def _get_time_from_seconds(self, seconds):


Less lines:

days, seconds = divmod(seconds, 86400) hours, seconds = divmod(seconds, 3600) minutes, seconds = divmod(seconds, 60) return self._etl_format.format(**locals())

foosel · 2017-06-07T14:24:56Z

octoprint_octobullet/__init__.py

+		minutes = 0
+		if seconds >= 86400:
+			days = int(seconds / 86400)
+			secionds = seconds % 86400


Typo, "secionds" instead of "seconds", this will produce wrong results.

foosel · 2017-06-07T14:25:26Z

octoprint_octobullet/__init__.py

 			access_token=None,
 			push_channel=None,
 			printDone=dict(
 				title="Print job finished",
 				body="{file} finished printing in {elapsed_time}"
+			),
+			printPartial=dict(


I think printProgress would be more fitting, or what do you think?

foosel · 2017-06-07T14:47:33Z

octoprint_octobullet/__init__.py

+		return self._etl_format.format(**locals())
+
+	#~~ PrintProgressPlugin
+	def on_print_progress(self, storage, path, progress):


I have to admit I don't fully understand the calculations going on in this method, but I have the feeling it is way too complicated for what it's supposed to achieve (regularly send an update to the user).

From what I understand your calculations are supposed to limit the amount of messages sent out due to the API limitations of the Pushbullet API, however, it's completely intransparent to me HOW it's supposed to do that and also it appears to do it in a very convulted way. Since in the end I'll be responsible of maintaining this, some explanation would be great :)

What I'd prefer here is splitting this method up to make operation a bit clearer. First of all determine if according to rate limiting you want to send a message in the first place. If not, abort and return. Once you've figured out you want to send a message, fetch the data you need for that, format it and go for it. Replace values you don't know yet with a placeholder ("unknown"?) instead of arbitrarily determined values. Also just fetch what you need from the data structure provided by the backend once instead of multiple times and create your own data structure. Makes things also way more readable due to less [][][] noise.

foosel · 2017-06-07T14:48:00Z

octoprint_octobullet/__init__.py

@@ -29,10 +33,106 @@ def __init__(self):
 		self._bullet = None
 		self._channel = None
 		self._sender = None
+		self._pc_interval = 5


That's a very cryptic identifier, what does pc stand for here?

foosel · 2017-06-07T14:50:54Z

octoprint_octobullet/templates/octobullet_settings.jinja2

@@ -26,6 +26,17 @@
            <input type="text" class="input-block-level" data-bind="value: settings.settings.plugins.octobullet.push_channel">
        </div>
    </div>
+    {% trans %}<p>
+        You can define a time interval (in minutes) here for regular update messages as the print progresses. Set to 0 to prevent updates. First and last messages will usually not be sent. Be aware of the monthly message limit applied to free PushBullet accounts.


Setting quiet time to 0 turns stuff off? I find this unintuitive. At least UI wise it should maybe rather be displayed as a toggle switch (which internally sets that value to 0 to signal it's disabled) which when toggled on allows setting the quiet time (> 1).

Also, I can't help myself, but I think "quiet_time" for something that is actually a "regular progress report interval" is a very unintuitive name?

New config added.

foosel · 2017-06-07T14:54:35Z

octoprint_octobullet/__init__.py

@@ -45,7 +145,7 @@ def on_settings_load(self):
 		data = octoprint.plugin.SettingsPlugin.on_settings_load(self)

 		# only return our restricted settings to admin users - this is only needed for OctoPrint <= 1.2.16
-		restricted = ("access_token", "push_channel")
+		restricted = ("quiet_minutes", "access_token", "push_channel")


I think we don't need to restrict access to "quiet_minutes", it's not really sensitive information, right?

foosel · 2017-06-07T14:54:48Z

octoprint_octobullet/__init__.py

 			)
 		)

 	def get_settings_restricted_paths(self):
 		# only used in OctoPrint versions > 1.2.16
-		return dict(admin=[["access_token"], ["push_channel"]])
+		return dict(admin=[["quiet_minutes"], ["access_token"], ["push_channel"]])


I think we don't need to restrict access to "quiet_minutes", it's not really sensitive information, right?

tsh-xx · 2017-06-07T21:59:02Z

Thanks for the review - should be easy enough to fix with those pointers...

foosel · 2017-06-23T12:32:23Z

Sorry, somehow Github didn't notify me about the fact that you pushed changes. I'll take a look ASAP (probably early next week though).

tsh-xx · 2017-06-23T14:24:18Z

I still have to update the more functional parts, may find time this weekend.

foosel · 2017-06-23T14:30:24Z

Ah, ok. In that case, could you drop me a quick comment here when you have done that? :)

Regular messages now generated based on time, not percent done. Config flag to disable regular messages Simplified generation of data to display

tsh-xx · 2017-06-26T06:51:48Z

OK, it should be worth another look now - it's all a lot simpler. I'm in two minds about if it's necessary to update the README to describe this feature, given that the config page is more visible to the end user.

foosel · 2017-07-12T08:42:15Z

Merged, slightly refactored and pushed as part of 0.1.9 together with some other things. Thanks :)

warbeerocks1 · 2017-07-13T20:32:03Z

Thank you

…

On Jul 13, 2017 4:41 AM, "Gina Häußge" ***@***.***> wrote: Just realized, I forgot to comment: merged, released, thanks :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#16 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AcIQXa5XXOSHZGsB1vB-1U44k7Duxyn7ks5sNfRpgaJpZM4NuQhn> .

Add sending regular progress messages

674dcfb

Default interval is 30 min (can be changes in the configuration page) First and last messages should be suppressed (based on estimated print time) Incremented revision

foosel requested changes Jun 7, 2017

View reviewed changes

Simple style cleanups, fix typos, moved messages to debug not info

9362654

Restructure to address review comments

adbc739

Regular messages now generated based on time, not percent done. Config flag to disable regular messages Simplified generation of data to display

foosel merged commit adbc739 into OctoPrint:master Jul 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sending regular progress messages #16

Add sending regular progress messages #16

tsh-xx commented Jun 2, 2017

prahjister commented Jun 2, 2017

foosel commented Jun 2, 2017

foosel left a comment

foosel Jun 7, 2017

tsh-xx Jun 26, 2017

foosel Jun 7, 2017

foosel Jun 7, 2017

foosel Jun 7, 2017

foosel Jun 7, 2017

foosel Jun 7, 2017

foosel Jun 7, 2017

foosel Jun 7, 2017

tsh-xx Jun 26, 2017

foosel Jun 7, 2017

foosel Jun 7, 2017

tsh-xx commented Jun 7, 2017

foosel commented Jun 23, 2017

tsh-xx commented Jun 23, 2017

foosel commented Jun 23, 2017

tsh-xx commented Jun 26, 2017

foosel commented Jul 12, 2017

warbeerocks1 commented Jul 13, 2017 via email


		return currentData

		def _get_time_from_seconds(self, seconds):

Add sending regular progress messages #16

Add sending regular progress messages #16

Conversation

tsh-xx commented Jun 2, 2017

prahjister commented Jun 2, 2017

foosel commented Jun 2, 2017

foosel 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

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

tsh-xx commented Jun 7, 2017

foosel commented Jun 23, 2017

tsh-xx commented Jun 23, 2017

foosel commented Jun 23, 2017

tsh-xx commented Jun 26, 2017

foosel commented Jul 12, 2017

warbeerocks1 commented Jul 13, 2017 via email