153 error notification #479

numeron5 · 2014-02-08T23:38:42Z

adding notifications bar

numeron5 · 2014-02-08T23:40:56Z

I haven't removed the pop up message ... just wanted to hear from you guys if this could be a way to notify the user

dideler · 2014-02-09T01:14:44Z

@numeron5 that style of notification works IMO. But for a more general notification system it may not work as well since the space is limited and other UIs in Freeseer have different layouts. But leave it as is until we hear more feedback from others.

dideler · 2014-02-09T01:26:42Z

src/freeseer/frontend/record/record.py

@@ -91,6 +91,10 @@ def __init__(self, profile, config):
        self.timer = QtCore.QTimer(self)
        self.timer.timeout.connect(self.update_timer)

+        # Set variables for Warning message
+        self.warning_message_off = True


This variable name can be improved IMO. It's a bit confusing because a "positive" value (True) is being assigned to a "negative" flag (off). Something such as display_warning_message = False is easier to read. It's usually better to use the "positive" option for flags or conditionals, it's easier to read.

FYI, it may seem pedantic pointing out stuff like this, but good naming makes a codebase much easier to understand. Related: http://programmers.stackexchange.com/a/41850/14467

zxiiro · 2014-02-09T05:00:47Z

I like the mockup. Don't worry about adding a notification to the configuration tool or the talk editor. I don't think notifications make sense in those UIs.

mtomwing · 2014-02-19T21:20:05Z

src/freeseer/frontend/record/record.py

+            self.display_warning_message = False
+        if not self.display_warning_message and ((self.remaining_disk_space[1] == 'GB' and \
+                                             float(self.remaining_disk_space[0]) < self.low_space_threshold) or \
+                                            (self.remaining_disk_space[1] == 'MB' or self.remaining_disk_space[1] == 'KB')):


Consider using intermediate variables to store the parts of the condition.

zxiiro · 2014-02-19T22:18:11Z

I think you have Disk Space and Error notification issues combined in this one pull request. To be honest I would prefer if you made them separate pull requests.

If we can get the Notification issue resolved quickly and merged in you can then use it as a base for your Disk Space issue. How about we up the priority of this bug and try to get this resolved quickly since I think a few projects depend on this. What do you think?

numeron5 · 2014-02-19T23:30:14Z

I agree, sorry about that.

I removed the warning message code. I will commit to it's corresponding branch as soon as I merge.

zxiiro · 2014-02-25T03:03:48Z

Can you rebase?

You have files in code review that is not part of your project. This is likely because you merged rather than rebased.

mtomwing · 2014-02-25T03:37:03Z

As it stands right now, your NotificationList is essentially an ordered dictionary - see collections.OrderedDict. You should consider instead making some sort of notification manager that would:

maintain a collection of notifications (using OrderedDict)
have some sort of event-driven callback system (e.g. GUI can register a callback for the 'warning-added' event which will be called when someone does manager.add_warning(notification))

This will also allow you to remove a lot of the notification logic from the GUI code.

jshyeung · 2014-02-27T04:38:10Z

src/freeseer/frontend/record/NotificationList.py

+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+
+class NotificationList:


You might want to look at python queue implementation.

numeron5 · 2014-03-01T00:27:05Z

I believe I made a mistake ... sorry

zxiiro · 2014-03-07T02:06:31Z

src/freeseer/frontend/qtcommon/NotificationManager.py

@@ -0,0 +1,30 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-


Where's the license header?

zxiiro · 2014-03-07T04:40:18Z

To be honest I was expecting the implementation to allow the display of multiple WARNING messages and ERROR messages as we've discussed numerous times. This was the whole reason I requested that you created a Notification Queue so that we could store multiple notifications at the same time. It's great that you were able to implement this queue but I think you completely missed the purpose of this queue which would enable you the ability to display multiple notifications at the same time.

Personally for the UI component I would create a new QWidget, this would be a basic container which can hold multiple QLabels, each QLabel corresponding to a notification. With this you will be able to display multiple notifications at the same time. The QLabels will need to be created dynamically at runtime. At runtime you will likely store some sort of key:value pair kinda list (I'm not sure how so you'll have to research what the best option is) which allows you to link a notification from the Queue back to the UI component it represents. This list will allow you to delete notifications that are no longer valid.

Hope this helps.

AnTuanHa · 2014-03-09T05:38:00Z

src/freeseer/frontend/qtcommon/NotificationManager.py

+
+# freeseer - vga/presentation capture software
+#
+#  Copyright (C) 2011, 2014  Free and Open Source Software Learning Centre


This file is new, so the copyright should only be 2014, not 2011 to 2014.

… MB.

…eseer starts recording below the threshold. - added notifications bar to record GUI in order to display warnings and errors to the user

…anch)

…FreeseerApp, added call bacl functions

…ing warnings

numeron5 · 2014-03-11T08:01:02Z

ok, at the moment I have a QWidget that stores all the notifications. The QWidget is not resizeable and the user is allowed to close it (this is not ideal but at the moment but, I just want a bit of feedback before polishing up the widget). It has two buttons, and up and down button which allows the user to traverse the notification queue.

I've done the implementation so that, only the add_warning, add_error and delete_notification methods should be used on the front end side.

I've only debugged the addition and deletion of warnings. I'll finish debugging the addition and deletion of errors tomorrow.

mtomwing · 2014-03-11T19:28:28Z

So you went from having a somewhat modular frontend/backend to having all your code in a bloated frontend component? It would be better if the notification system and presentation were separate.

mtomwing · 2014-03-11T19:32:40Z

src/freeseer/frontend/qtcommon/NotificationPanel.py

+        self.update()
+
+    def add_error(self, name, message):
+        self.n_manager[name] = message


What happens if I re-use the name of an existing error or warning? Wouldn't it get overwritten?

mtomwing · 2014-03-11T19:38:08Z

What reasoning do you have for using an OrderedDict instead of a list or some kind of queue? You seem to be using an index (which isn't the key in the dict) to look up your notifications which leads to me believe the wrong data structure was used.

numeron5 · 2014-03-11T23:38:05Z

yes, if you re-use a name, the notification message will get over written. But this should never happen since each notification is unique.
I agree, I should use index instead of keyword
The reason why I'm using an ordered dictionary is because, I want a list of tuples such that: (notification ID : notification message) are linked. that way It allows for the direct deletion using the notification ID (or keyword). The reason why I'm using an index to traverse the dictionary is only GUI purposes ... it has nothing to do with the actual queue implementation.

mtomwing · 2014-03-12T00:51:10Z

I'm not sure if this is the best assumption as this requires all contributors to know about all possible notification names so that they don't collide. Maybe it would be better generate a unique identifier instead and let the caller use that to delete their notification in the future?
If you go the unique identifier route then perhaps an OrderedDict could work. But without unique keys, I don't think it makes sense. Additionally, the GUI shouldn't necessarily have to know about the existence of this "queue".

numeron5 · 2014-03-13T01:09:32Z

here is what I have so far for the notification system design:

dideler · 2014-03-13T03:42:24Z

Thanks for the diagrams @numeron5. I'll take a better look at it tomorrow and leave some feedback.

numeron5 · 2014-03-28T02:38:00Z

I've created a new pull request where the implementation of the notification system uses observer pattern.

link to the new pull request: #530

dideler reviewed Feb 9, 2014
View reviewed changes

mtomwing reviewed Feb 19, 2014
View reviewed changes

jshyeung reviewed Feb 27, 2014
View reviewed changes

dideler added this to the Open Academy / UCOSP milestone Mar 1, 2014

zxiiro reviewed Mar 7, 2014
View reviewed changes

AnTuanHa reviewed Mar 9, 2014
View reviewed changes

numeron5 added 9 commits March 11, 2014 03:50

Alerts the user that space is low when the remaining disk space is 15…

25cfeba

… MB.

fixed style problems and bugs from my previous commit

0c21180

changed the threshold to 10 GB

a0d9d98

- fixed a bug where the low disk space warning wouldn't show when fre…

e8e4ce4

…eseer starts recording below the threshold. - added notifications bar to record GUI in order to display warnings and errors to the user

added mutator functions for notification bar

183fe02

correcting style errors

07bf28c

Removed Warning message code (will be committed to it's respective br…

10e2db5

…anch)

Local example of notification queue

371ae01

modified notification list class

b5ec431

numeron5 added 3 commits March 11, 2014 03:50

used ordered dictionary for notification queue, initialized queue in …

335fcc7

…FreeseerApp, added call bacl functions

placed notification manager in record.py

8c09512

deleted Notification Manager, Added Notification Panel, debugged- add…

c0d4191

…ing warnings

mtomwing reviewed Mar 11, 2014
View reviewed changes

numeron5 closed this Mar 28, 2014

zxiiro mentioned this pull request Sep 21, 2014

Suggested modification: Play video button moved into a popup dialog #586

Closed

dideler mentioned this pull request Oct 19, 2014

Error Notification #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

153 error notification #479

153 error notification #479

numeron5 commented Feb 8, 2014

numeron5 commented Feb 8, 2014

dideler commented Feb 9, 2014

dideler Feb 9, 2014

zxiiro commented Feb 9, 2014

mtomwing Feb 19, 2014

zxiiro commented Feb 19, 2014

numeron5 commented Feb 19, 2014

zxiiro commented Feb 25, 2014

mtomwing commented Feb 25, 2014

jshyeung Feb 27, 2014

numeron5 commented Mar 1, 2014

zxiiro Mar 7, 2014

zxiiro commented Mar 7, 2014

AnTuanHa Mar 9, 2014

numeron5 commented Mar 11, 2014

mtomwing commented Mar 11, 2014

mtomwing Mar 11, 2014

mtomwing commented Mar 11, 2014

numeron5 commented Mar 11, 2014

mtomwing commented Mar 12, 2014

numeron5 commented Mar 13, 2014

dideler commented Mar 13, 2014

numeron5 commented Mar 28, 2014

153 error notification #479

153 error notification #479

Conversation

numeron5 commented Feb 8, 2014

numeron5 commented Feb 8, 2014

dideler commented Feb 9, 2014

dideler Feb 9, 2014

Choose a reason for hiding this comment

zxiiro commented Feb 9, 2014

mtomwing Feb 19, 2014

Choose a reason for hiding this comment

zxiiro commented Feb 19, 2014

numeron5 commented Feb 19, 2014

zxiiro commented Feb 25, 2014

mtomwing commented Feb 25, 2014

jshyeung Feb 27, 2014

Choose a reason for hiding this comment

numeron5 commented Mar 1, 2014

zxiiro Mar 7, 2014

Choose a reason for hiding this comment

zxiiro commented Mar 7, 2014

AnTuanHa Mar 9, 2014

Choose a reason for hiding this comment

numeron5 commented Mar 11, 2014

mtomwing commented Mar 11, 2014

mtomwing Mar 11, 2014

Choose a reason for hiding this comment

mtomwing commented Mar 11, 2014

numeron5 commented Mar 11, 2014

mtomwing commented Mar 12, 2014

numeron5 commented Mar 13, 2014

dideler commented Mar 13, 2014

numeron5 commented Mar 28, 2014