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

[WIP] Add a feedback dialog box #93

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
b25c109
Initial commit
Jun 13, 2019
faf06fc
Don't use relative imports
Jun 13, 2019
644b52a
Specifying bot OAuth token should be user-developer's concern
Jun 13, 2019
9dc5122
Unify image interface
Jun 13, 2019
8e9dcc8
Further simplify image interface
Jun 14, 2019
f2fb60b
Modify import
Jun 17, 2019
fc6e146
Use single name field and use property for message
Jun 17, 2019
4057d6d
Fix bug in msg getter
Jun 17, 2019
2afabb7
Added comments
Jun 17, 2019
b94d489
Added comment regarding asynchronous functionality.
Jun 18, 2019
b6e7b01
Refactor send function
Jun 19, 2019
82bd9dc
Better error handling and notification dialogs in controller.
Jun 19, 2019
40bcab8
Refactor logic and introduce logging.
Jun 20, 2019
a5471da
Improve comments
Jun 20, 2019
2a304a8
Use HasRequiredTraits
Jun 21, 2019
beb3813
Add tests.
Jun 21, 2019
7c407d7
Bugfix in example
Jun 21, 2019
40f47fd
Use custom style for Description field
Jun 21, 2019
b7da2c2
PEP8 compliance
Jun 21, 2019
0db10aa
Put error dialog test in a helper function.
Jun 24, 2019
f60992c
Compress image inside send function.
Jun 25, 2019
1a61255
Fix incorrect syntax for metadata dependence.
Jun 25, 2019
b34a768
Add test to ensure files_upload is called correctly
Jun 27, 2019
ef1a643
Remove trailing _
Jun 27, 2019
1ee8f74
Remove trailing _
Jun 27, 2019
d7ffe07
Move example file
Jun 27, 2019
e659e49
Add comments + fix typos
Jun 27, 2019
133a35f
Add README
Jun 28, 2019
12691b6
Fix typos in README
Jun 28, 2019
d651bf8
Fixed typo in function name
Jun 28, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file.
113 changes: 113 additions & 0 deletions apptools/feedback/feedbackbot/model.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
"""
This module implements a class that provides logic for a simple plugin
for sending messages to a developer team's slack channel.
"""

import io

import numpy as np
import slack
import aiohttp
from PIL import Image
from traits.api import (
HasTraits, Str, Property,
Int, Array, Bytes, String,
cached_property, on_trait_change)


class FeedbackMessage(HasTraits):
"""Model for the feedback message.

Notes
-----
The user-developer must specify the slack channel that the message must be
sent to, as well as provide raw screenshot data.

"""

first_name = Str(msg_meta=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please give a comment for all trait definitions, eg. #: The first name of the user.

A secondary comment is that it's probably easier to have just one name field unless there is some compelling reason to split it.


last_name = Str(msg_meta=True)

#: Name of the client organization.
organization = Str(msg_meta=True)

# TODO: Slack supports some markdown in messages, provide
# some details here.
#: Main body of the feedback message.
description = Str(msg_meta=True)

#: The target slack channel that the bot will post to, must start with #.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Channels" and "tokens" are not part of the concept of a "message". They are part of the concept of a (particular) delivery system and so should be factored out into their own class (see main review comment).

channels = String(minlen=2, regex='#.*')

#: OAuth token for the slackbot, must be provided by the user-developer.
token = Str

#: The final slack message that will be posted.
msg = Str

#: The screenshot pixel data in raw bytes. Note that RGB[A] ordering is assumed.
img_bytes = Bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a numpy array (ie. image = Array(shape=(None, None, 3)) for an RGB image, (None, None, 4) for RGBA). It is easy to convert to a Pillow Image from an array, and you don't need to store auxiliary information like the height and width.

You are using Chaco and/or Enable here, so numpy is already a dependency.


#: The screenshot width in pixels.
img_w = Int

#: The screenshot height in pixels.
img_h = Int

@on_trait_change('+msg_meta')
Copy link
Contributor

Choose a reason for hiding this comment

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

For this it makes more sense to use a Property instead of a trait-change handler.

def _update_msg(self):

feedback_template = 'Name: {first} {last}\n' \
+ 'Organization: {org}\nDescription: {desc}'

self.msg = feedback_template.format(
first=self.first_name,
last=self.last_name,
org=self.organization,
desc=self.description)

def send(self):
""" Send feedback message and screenshot to slack. """

client = slack.WebClient(token=self.token,
timeout=5,
ssl=True)

# Compress image into PNG format using an in-memory buffer.
#img = Image.fromarray(self.img_data, mode='RGB')
img = Image.frombytes('RGBA', (self.img_w, self.img_h), self.img_bytes)

buf = io.BytesIO()

img.save(buf, 'PNG')
buf.seek(0)

try:

# Send message.
response = client.files_upload(
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice that Slack is using aiohttp, but we are in an Qt application which has no notion or knowledge of Python's async stuff. If you're lucky the application will just block here and the window will freeze while you upload the files. If you are unlucky the async event loop and the Qt event loop will not play nicely, and you will have to require all users of your library to use something like asyncqt (which is a big ask as ETS is not built with that in mind).

For UX reasons you want to keep the UI responsive while the files are uploading; it would be awesome if you can give progress; but you don't want to impose major architectural/technology constraints on the users of your library.

This is deep water here. Be careful.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. Slack provides a flag which lets users decide whether they want to use the asynchronous feature or not. The flag is False by default, ensuring that HTTP requests are blocking (only one HTTP request is added to an event loop, which is run to completion before the function returns).

channels=self.channels,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: indentation.

Please run flake8 (or equivalent) over your code.

initial_comment=self.msg,
filetype='png',
filename='screenshot.png',
file=buf)

except slack.errors.SlackApiError as error:

print(
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use print, instead use logging. Standard procedure for logging in a library like this is to have at the top of the file a

import logging

logger = logging.getLogger(__name__)

and then here you would use logger.error and/or logger.exception to record the error.

'Message sent successfully,'
+ ' but received the following error from Slack:')
print(error)
raise

except aiohttp.client_exceptions.ClientConnectorError as error:

print('Message not sent.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here about logging.

print(error)
raise

else:

print('Message sent successfully!')
Copy link
Contributor

Choose a reason for hiding this comment

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

And here, but this would be logger.info.


117 changes: 117 additions & 0 deletions apptools/feedback/feedbackbot/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
"""
This module provides some helper functions for the feedback plugin. These
cfarrow marked this conversation as resolved.
Show resolved Hide resolved
functions are designed to be used by the user-developer so that the feedback
plugin can be used in their application.
"""

from PyQt4.QtGui import QPixmap
Copy link
Contributor

Choose a reason for hiding this comment

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

Please import from pyface.qt.QtGui so this is more portable.

import numpy as np

def take_screenshot_qimg(info):
""" Take screenshot of an active GUI widget.

Parameters
----------
info: traitsui.UIInfo
The UIInfo instance which contains the active widget.

Returns:
--------
qimg: PyQt4.QtGui.QImage
Screenshot image as PyQt4.QtGui.QImage instance.

"""

pixmap = QPixmap.grabWidget(info.ui.control)

qimg = pixmap.toImage()

return qimg

def get_raw_qimg_data(qimg):
""" Get raw image data (BGR[A] values, and size in pixels).

Parameters:
qimg: PyQt4.QtGui.Qimage instance

Returns:
--------
bytes
Raw bytes ordered as BGR[A]. Alpha channel is included if available.
int
Image height in pixels.
int
Image width in pixels.

"""

qbits = qimg.bits()

num_channels = qimg.depth() // 8

qbits.setsize(qimg.width() * qimg.height() * num_channels)

return [qbits.asstring(), qimg.height(), qimg.width()]

def bgr_bytes_to_rgb_bytes(bgr_bytes, height, width):
""" Convert BGR[A] bytestring to RGB[A] bytestring.

Note
----
This function is designed to convert the BGR[A]-ordered
bytestring of a PyQt4.QtGui.QImage into RGB[A] ordering.
An alpha-channel is not necessary, but will be handled if provided.

Parameters
----------
bgr_bytes: bytes
BGR[A]-ordered bytestring.

height: int
Height of image in pixels.

width: int
Height of image in pixels.

Returns
-------
bytes
RGB[A]-ordered bytes

"""

bgr_mat = bytes_to_matrix(bgr_bytes, height, width)

num_channels = bgr_mat.shape[2]

if num_channels == 3:

new_channel_idx = [2, 1, 0]

elif num_channels == 4:

new_channel_idx = [2, 1, 0, 3]

else:

raise ValueError(
"Image has {} channels. Expected 3 or 4.".format(num_channels))

return bgr_mat[..., new_channel_idx].tobytes()

def bytes_to_matrix(bytes_str, height, width):

return np.ascontiguousarray(np.frombuffer(
bytes_str, dtype=np.uint8).reshape(height, width, -1))

def bytes_to_buffer(bytes_str, height, width, fmt):

img = Image.frombytes('RGBA', (width, height), bytes_str)

buf = io.BytesIO()

img.save(buf, fmt)
buf.seek(0)

return buf

97 changes: 97 additions & 0 deletions apptools/feedback/feedbackbot/view.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
"""
This model implements UI classes and logic for a plugin that enables
clients to send feedback messages to a developer team's slack channel.
"""

from traits.api import Property, Instance
from traitsui.api import (
View, Group, Item, Action,
Label, Controller)
from traitsui.menu import CancelButton
from chaco.api import Plot, ArrayPlotData
from enable.api import ComponentEditor

from .model import FeedbackMessage
from .utils import bytes_to_matrix

# ----------------------------------------------------------------------------
# TraitsUI Actions
# ----------------------------------------------------------------------------

send_button = Action(name='Send', action='send',
enabled_when='controller._send_enabled')

# ----------------------------------------------------------------------------
# TraitsUI Views
# ----------------------------------------------------------------------------

#: Primary view for the feedback message.
feedback_msg_view = View(
Label('Enter feedback here. All fields are mandatory.'),
Group(
Group(
Item('first_name'),
Item('last_name'),
Item('organization',
tooltip='Enter the name of your organization.'),
Item('description',
tooltip='Enter feedback.',
height=200,
springy=True)),
Group(
Item('controller.screenshot_plot',
editor=ComponentEditor(),
show_label=False)),
orientation='horizontal'),
buttons=[CancelButton, send_button],
width=800,
resizable=True)


# ----------------------------------------------------------------------------
# TraitsUI Handler
# ----------------------------------------------------------------------------

class FeedbackController(Controller):
cfarrow marked this conversation as resolved.
Show resolved Hide resolved
"""Controller for FeedbackMessage.

The Controller allows the client user to specify the feedback and preview
the screenshot.

"""

model = Instance(FeedbackMessage)

#: Chaco plot to display the screenshot.
screenshot_plot = Instance(Plot)

#: Property that decides whether the state of the message is valid
# for sending.
_send_enabled = Property(depends_on='[+msg_meta]')

trait_view = feedback_msg_view

def _screenshot_plot_default(self):
""" Plots screenshot in Chaco from RGB data. """

# Reverse rows of model.img_data so that the img_plot looks right

cfarrow marked this conversation as resolved.
Show resolved Hide resolved
img_data = bytes_to_matrix(
self.model.img_bytes, self.model.img_h, self.model.img_w)

plotdata = ArrayPlotData(img_data=img_data[::-1, ...])
plot = Plot(plotdata)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use Chaco for this, instead use an Enable Image component: https://github.com/enthought/enable/blob/master/enable/primitives/image.py . Much simpler, no worrying about extra plot details, see https://github.com/enthought/enable/blob/master/examples/enable/image_draw.py for example.

plot.img_plot('img_data', hide_grids=True)

plot.border_visible = False
plot.x_axis = None
plot.y_axis = None

return plot

def _get__send_enabled(self):
""" Logic to check if message is valid for sending. """

return self.model.first_name and self.model.last_name \
and self.model.organization and self.model.description

Loading