Added new popwindow module in sugar3.graphics #313

devAbnull · 2016-04-04T21:38:32Z

@samdroid-apps , as discussed I have designed the pop-up model here.
Use of this can be seen in my PR in sugar repo.
Link-> sugarlabs/sugar#672

samdroid-apps · 2016-04-04T21:52:20Z

src/sugar3/graphics/popwindow.py

+
+class PopWindow(Gtk.Window):
+    """
+    UI interface for activity Pop-up Windows


Please write some docs for this. Here is an example of the format: https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/256/files#diff-74308e345a99a21f5fbfc47f0dbcc070R106

quozl · 2016-04-04T22:44:06Z

Please add more explanation to the commit description and force push. I've no idea what the intention of the patch is.

quozl · 2016-04-04T22:44:53Z

src/sugar3/graphics/Makefile.am

@@ -16,6 +16,7 @@ sugar_PYTHON =              \
 	palettemenu.py          \
 	palettewindow.py        \
 	panel.py                \
+        popwindow.py            \


Probably mismatch of tab and space characters in this line compared to the lines either side, causing diff to show slight indentation.

devAbnull · 2016-04-05T19:10:11Z

@samdroid-apps wrote docs for class and methods implemented.
@quozl commit message changed.

quozl · 2016-04-05T20:24:11Z

@AbrahmAB, thanks for changing the commit message, but perhaps I didn't say enough about it.

You wrote:

Refactoring the Pop-up windows in sugar, implemented a basic modal for pop-up windows

the patch looks like it adds a new module, but the message says it is a refactoring, (as if you mixed up the commit message with Refactor ViewSource as per sugarlabs/sugar-toolkit-gtk3#313 sugar#672),
there's no link in the commit message to the corresponding commit hash in the pull request of the other repository,
the first line of the message is too long, it wraps in displays, please use a shorter first line, then a blank line, then a paragraph of narrative or bullet points if necessary to explain,
sugar is redundant information, we know from context that this is a patch for Sugar,
look for ways to shorten the text, e.g. refactoring could be refactor, and pop-up windows need only be mentioned once.

Hope that helps! The purpose of commit messages is to communicate to other programmers now and in the future. Programmers who are doing a bisection to find the cause of a regression need as much information about the change as possible, yet in a form that is readily taken up.

samdroid-apps · 2016-04-05T21:00:28Z

src/sugar3/graphics/popwindow.py

+    These pop-up windows don't cover the whole screen.
+    They contain canvas content, alerts messages, a tray and a
+    toolbar.
+    """


Please document the constructor kwargs in this section, eg:

Kwargs: title (type): what is it set_titlebox (type): what is it window_xid (type): what is it

devAbnull · 2016-04-05T22:25:23Z

@samdroid-apps I have made the following changes in the code

Added document the constructor kwargs for PopWindow class.
Improved docs for get_vbox and get_scroll_window.
Replaced variable _sample_window with _scroll_window.
Removed the _add_events and copy of Gtk.Window methods like self._set_decorated etc.
Changed the method name _add_widget to add_widget in TitleBox.

devAbnull · 2016-04-05T22:34:11Z

@quozl Thanks for your pointing out main points while writing a commit message.
I have changed the commit message again. Hope, that would be more informative! :)

…-toolkit-gtk3#313) in sugar-toolkit-gtk3

…in sugar-toolkit-gtk3

devAbnull · 2016-04-05T22:50:39Z

Oops! I made some typo error while changing the commit message of my PR in sugar repo having reference of this PR, for which I had to commit --amend -m again. I apologize for multiple reference links above :(.

samdroid-apps · 2016-04-15T22:01:14Z

The api looks good. I'm afk so I cant test it, but I'm sure that @quozl can merge it if he is satisfied.

quozl · 2016-04-15T23:14:49Z

Happy to merge into my fork, but I don't think I have community support to merge into Sugar Labs.

i5o · 2016-04-18T17:17:45Z

I will test ASAP.

Hi everyone btw.

samdroid-apps · 2016-04-20T21:50:02Z

src/sugar3/graphics/popwindow.py

+    HALF_WIDTH = ((Gdk.Screen.height() - style.GRID_CELL_SIZE * 5)/2,
+                  (Gdk.Screen.height() - style.GRID_CELL_SIZE * 3))
+
+    def __init__(self, set_titlebox=True, window_xid=None, **kwargs):


This patch is looking great. Just 2 little changes that might polish it up:

Could we remove the "set_titlebox" property? Most people will just use the titlebox, and it can always be removed via get_titlebox().hide().

Could you add a property or constructor argument that takes one of the size constants?

Adding props for size(having tuple of width, height) or separate for width and height.
I would prefer the separate properties. What are your suggestions @samdroid-apps ?

Having just one prop would be very good developer experience:

popup = PopWindow(PopWindow.FULLSCREEN)

On Thu, Apr 21, 2016 at 7:55 AM, Abhijit Patel
notifications@github.com wrote:

In src/sugar3/graphics/popwindow.py:

They contain canvas content, alerts messages, a tray and a

toolbar.

FULLSCREEN and HALF_WIDTH for setting size of the window.

Kwargs:

set_titlebox (bool): False if titlebox is not needed

window_xid (xlib.Window): xid of the parent window

"""

FULLSCREEN = (Gdk.Screen.width() - style.GRID_CELL_SIZE * 5,

Gdk.Screen.height() - style.GRID_CELL_SIZE \* 3)

HALF_WIDTH = ((Gdk.Screen.height() - style.GRID_CELL_SIZE *
5)/2,

(Gdk.Screen.height() - style.GRID_CELL_SIZE \* 3))

def init(self, set_titlebox=True, window_xid=None,
**kwargs):
Adding props for size(having tuple of width, height) or separate for
width and height.
I would prefer the separate properties. What are your suggestions
@samdroid-apps ?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

devAbnull · 2016-04-20T22:25:04Z

Changes made:

Removed set_title_box property.
Added constructor argument size .

samdroid-apps · 2016-04-24T11:53:34Z

src/sugar3/graphics/popwindow.py

+    def __init__(self):
+        ToolbarBox.__init__(self)
+
+        self.close_button = ToolButton(icon_name='dialog-cancel')


I was just testing as I was integrating this into bibliography activity. Can you set the "tooltip=_('Close')", to be consistent with existing popups?

tooltip set 👍

samdroid-apps · 2016-05-24T06:31:33Z

I will do a code review soon, but 1st there are some issues to fix:

Toolbar adding padding - the label should be on the very right and the buttons on the very left

Left buttons are in the wrong order, padding is an issue

samdroid-apps · 2016-05-24T06:32:52Z

src/sugar3/graphics/popwindow.py

+        # the child class to display the window after modifications
+        # like chaninging window size, decorating, changing position.
+
+        #self.show()


Please remove this commented code

devAbnull · 2016-06-04T20:35:00Z

Forced push with changes:

Fixed padding issue :) used Gtk.Toolbar instead of sugar3.graphics.ToolbarBox
Removed commented codes
Added docs string with example as per the syntax
GObject.Property for size , title_box, vbox, TitleBox.title
Removed add_widget method in TitleBox class

samdroid-apps · 2016-06-05T00:29:35Z

Awsome! Merged! I can't wait to refactor my activities to use this.

iamutkarshtiwari · 2016-06-13T12:02:19Z

src/sugar3/graphics/popwindow.py

+        # like chaninging window size, decorating, changing position.
+
+    def set_size(self, size):
+        width, height = size


This way of assigning two variable with one value isn't right. It should be somewhat like this width, height = size_x, size_y

No, you pass a tuple;

tuple = (1, 200) a, b = tuple

The argument for this function is tuple of size itself!

Ah.. Got it

iamutkarshtiwari · 2016-06-15T08:51:27Z

I am using this module for my 'Screenshot' popup. But whenever I click a snapshot, before the popup shows up on the screens, I see a grey rectange appear for a small fraction of second (which also get's captured in my screenshot'). You can see it in the attached screenshot below -

Captured screenshot below -

quozl · 2016-06-15T23:20:23Z

Possibly a window realisation and size negotiation race.

The early stages of PopWindow.__init__ may be forcing Gtk to create the Gdk window in order to set the attributes requested. However, at the time this happens, there are no children in the window to negotiate a size. The children are added later.

You might experiment by moving parts of PopWindow.__init__ around; add the child widgets as soon as possible. Then call self.connect for the realize event. Then make the other calls. Then call self.connect for the remaining events.

Or, perhaps @AbrahmAB's design feature of not calling self.show (see comments at end of __init__) may need to be revisited.

devAbnull · 2016-06-16T17:08:52Z

Hi Utkarsh,
I tried testing your patch but its showing the following traceback in my log
Traceback (most recent call last):
File "/home/broot/sugar-build/build/out/install/lib/python2.7/site-packages/jarabe/frame/devicestray.py", line 34, in __init__
locals(), [module_name])
File "/home/broot/sugar-build/build/out/install/share/sugar/extensions/deviceicon/display.py", line 37, in <module>
from jarabe.screenshotpanel.gui import ScreenshotPanel
ImportError: No module named screenshotpanel.gui

quozl · 2016-06-16T22:31:13Z

@AbrahmAB, the error shows file gui.py is missing; check that you have properly applied the patches in the pull request and that a file gui.py is in the screenshotpanel directory. See how sugarlabs/sugar@02636de adds that file.

devAbnull mentioned this pull request Apr 4, 2016

Refactor ViewSource as per sugarlabs/sugar-toolkit-gtk3#313 sugarlabs/sugar#672

Closed

samdroid-apps reviewed Apr 4, 2016
View reviewed changes

samdroid-apps added the bug label Apr 4, 2016

devAbnull force-pushed the popup branch from 7bce8d6 to 077299d Compare April 4, 2016 22:37

quozl reviewed Apr 4, 2016
View reviewed changes

devAbnull force-pushed the popup branch 2 times, most recently from 3ff1217 to bef7341 Compare April 5, 2016 19:04

devAbnull changed the title ~~Base modal for all pop-up windows~~ Refactoring the Pop-up windows in sugar, implemented a basic modal for pop-up windows Apr 5, 2016

devAbnull force-pushed the popup branch from bef7341 to 2bc8521 Compare April 5, 2016 19:24

samdroid-apps reviewed Apr 5, 2016
View reviewed changes

devAbnull force-pushed the popup branch from 2bc8521 to 12f2f50 Compare April 5, 2016 22:18

devAbnull force-pushed the popup branch from 12f2f50 to fc3b08a Compare April 5, 2016 22:30

devAbnull changed the title ~~Refactoring the Pop-up windows in sugar, implemented a basic modal for pop-up windows~~ Added new popwindow module in sugar3.graphics Apr 5, 2016

devAbnull added a commit to devAbnull/sugar that referenced this pull request Apr 5, 2016

Refactor ViewSource corresponds to PR [sugarlabs#313](sugarlabs/sugar…

fc74636

…-toolkit-gtk3#313) in sugar-toolkit-gtk3

devAbnull added a commit to devAbnull/sugar that referenced this pull request Apr 5, 2016

Refactor ViewSource corresponds to PR sugarlabs/sugar-toolkit-gtk3#313 …

44cd0bc

…in sugar-toolkit-gtk3

devAbnull added a commit to devAbnull/sugar that referenced this pull request Apr 5, 2016

Refactor ViewSource as per sugarlabs/sugar-toolkit-gtk3#313

a027b06

devAbnull force-pushed the popup branch from fc3b08a to 95a9829 Compare April 6, 2016 14:39

samdroid-apps reviewed Apr 20, 2016
View reviewed changes

devAbnull force-pushed the popup branch from 3d61134 to 2898fd3 Compare April 20, 2016 22:22

devAbnull force-pushed the popup branch from 2898fd3 to a78fe3b Compare April 20, 2016 22:47

samdroid-apps reviewed Apr 24, 2016
View reviewed changes

devAbnull force-pushed the popup branch from a78fe3b to 800ce1a Compare May 13, 2016 06:59

samdroid-apps reviewed May 24, 2016
View reviewed changes

Added new popwindow module in sugar3.graphics

a803876

devAbnull force-pushed the popup branch from 800ce1a to a803876 Compare June 4, 2016 20:27

samdroid-apps merged commit a803876 into sugarlabs:master Jun 5, 2016

iamutkarshtiwari reviewed Jun 13, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new popwindow module in sugar3.graphics #313

Added new popwindow module in sugar3.graphics #313

devAbnull commented Apr 4, 2016

samdroid-apps Apr 4, 2016

quozl commented Apr 4, 2016

quozl Apr 4, 2016

devAbnull commented Apr 5, 2016

quozl commented Apr 5, 2016

samdroid-apps Apr 5, 2016

devAbnull commented Apr 5, 2016

devAbnull commented Apr 5, 2016

devAbnull commented Apr 5, 2016

samdroid-apps commented Apr 15, 2016

quozl commented Apr 15, 2016

i5o commented Apr 18, 2016

samdroid-apps Apr 20, 2016

devAbnull Apr 20, 2016

samdroid-apps Apr 20, 2016

devAbnull commented Apr 20, 2016

samdroid-apps Apr 24, 2016

devAbnull May 13, 2016

samdroid-apps commented May 24, 2016

samdroid-apps May 24, 2016

devAbnull commented Jun 4, 2016

samdroid-apps commented Jun 5, 2016

iamutkarshtiwari Jun 13, 2016

samdroid-apps Jun 13, 2016

devAbnull Jun 13, 2016

iamutkarshtiwari Jun 14, 2016

iamutkarshtiwari commented Jun 15, 2016

quozl commented Jun 15, 2016

devAbnull commented Jun 16, 2016

quozl commented Jun 16, 2016

Added new popwindow module in sugar3.graphics #313

Added new popwindow module in sugar3.graphics #313

Conversation

devAbnull commented Apr 4, 2016

Choose a reason for hiding this comment

quozl commented Apr 4, 2016

Choose a reason for hiding this comment

devAbnull commented Apr 5, 2016

quozl commented Apr 5, 2016

Choose a reason for hiding this comment

devAbnull commented Apr 5, 2016

devAbnull commented Apr 5, 2016

devAbnull commented Apr 5, 2016

samdroid-apps commented Apr 15, 2016

quozl commented Apr 15, 2016

i5o commented Apr 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devAbnull commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdroid-apps commented May 24, 2016

Choose a reason for hiding this comment

devAbnull commented Jun 4, 2016

samdroid-apps commented Jun 5, 2016

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

iamutkarshtiwari commented Jun 15, 2016

quozl commented Jun 15, 2016

devAbnull commented Jun 16, 2016

quozl commented Jun 16, 2016