[Sugar-devel] [PATCH] Fixes to the record UI
Anish Mangal
anishmangal2002 at gmail.com
Wed Jun 9 00:25:58 EDT 2010
Hi,
I did reply to your mail earlier today, I'll paste my comments here ...
> Still unanswered questions from my last mail:
> There was probably a reason why it moved them off screen instead of hiding them.
> Do you have any idea what it is?
This comment in ui.py explains why
#we move offscreen to resize or else we get flashes on screen,
and setting hide() doesn't allow resize & moves
> If we are no longer moving windows offscreen, I would expect your
> patch to remove a load of code that does that, but I don't see it.
Incorporated in the updated patch. (And it has removed a load of code)
> Also, if you are hiding widgets, I don't see any reason to resize them to 1x1.
For some widgets, when I attempt to hide them, the UI doesn't work, so
I have to resize them to 1x1.
(I only resize only those widgets which I can't hide, so there is no redundancy)
> Comments for other reviewers: for me, the patch is now harder to
> review because it contains a load of unrelated changes renaming
> variables. I agree with the name changes, but perhaps best to be
> careful with feedback this way.
Sorry, I'll be careful to include separate changes in different
patches the next time.
> On IRC you said that there were some issues with your old patch when
> it is applied. Are they 100% resolved now?
The issue was concerning the "Fullscreen/Maximize icon" which gets
hidden. This has been fixed (and tested on soas/xo1/emulator-0.88).
>> def startTakingPhoto( self ):
>> self.setUpdating( True )
>> self.ca.glive.takePhoto()
>> + self.ca.ui.FULLSCREEN = False
>> + self.ca.ui.updateVideoComponents()
>
> This needs some explanation.
> We can be taking photos in full screen mode.
> There are also no changes in the video components when we are taking a photo.
>
> So these 2 lines seem wrong/unnecessary or indicative of a larger problem.
No problem here. This seemed like a minor quirk to me when compared
with recording videos in fullscreen. When we finish recording a video
in fullscreen mode, we are automatically taken out of fullscreen mode.
However in the case of taking photos, when we click the shutter, the
photo is captured but we remain in full screen mode. This small
snippet takes us out of fullscreen after capturing the photo. We can
totally do without this code as well and the UI would work just fine.
>> @@ -335,7 +335,7 @@ class UI:
>> self.tagsBuffer = gtk.TextBuffer()
>> self.tagsBuffer.connect('changed', self._tagsBufferEditedCb)
>> self.tagsField = gtk.TextView(self.tagsBuffer)
>> - self.tagsField.set_size_request( 100, 100 )
>> + self.tagsField.set_size_request( 50, 50 )
>> self.tagsPanel.pack_start(self.tagsField, expand=True)
>> self.infoBoxTopLeft.pack_start(self.tagsPanel, expand=True)
>
> Why this change?
When running record on smaller screen resolutions (or perhaps even xo
resolution), in the info view, the preview window slightly overlaps
with the tags and date text fields. This snippet attempts to fix that.
>> def getLoc( self, pos, full ):
>> @@ -1316,6 +1304,7 @@ class UI:
>> else:
>> #or, if there is no countdown, it might be because we are recording
>> self.clickShutter()
>> + self.progressWindow.updateProgress( 1, Constants.istrFinishedRecording )
>
> This change is also surprising to see.
Its a coding artifact :-). I removed many of them from the previous
patch but this one sneaked through. Its redundant code and the UI
works just wine without this. I'll remove this before committing.
Cheers,
Anish
On Wed, Jun 9, 2010 at 4:37 AM, Daniel Drake <dsd at laptop.org> wrote:
> On 8 June 2010 13:52, <anishmangal2002 at gmail.com> wrote:
>> Fix description:
>> The patch works by hiding or resizing (to size 1px by 1px) the
>> widgets not required in a particular view/mode. The updateVideoComponents
>> method has been modified to hide the widgets not required in a
>> particular view. Widgets that can't be hidden are resized to
>> 1 x 1 pixel.
>>
>> Additionally, this patch also fixes the naming of some variables
>> (s/butt/button/g).
>>
>> Tested successfully on sugar-emulator-0.88, soas-mirabelle and
>> xo1-f11-0.88.
>
>
> Still unanswered questions from my last mail:
> There was probably a reason why it moved them off screen instead of hiding them.
> Do you have any idea what it is?
>
>
> This is a great start but we should look towards that 'ideal fix' route.
> If we are no longer moving windows offscreen, I would expect your
> patch to remove a load of code that does that, but I don't see it.
> Also, if you are hiding widgets, I don't see any reason to resize them to 1x1.
>
> Comments for other reviewers: for me, the patch is now harder to
> review because it contains a load of unrelated changes renaming
> variables. I agree with the name changes, but perhaps best to be
> careful with feedback this way.
>
> On IRC you said that there were some issues with your old patch when
> it is applied. Are they 100% resolved now?
>
>> Signed-off-by: anishmangal2002 <anishmangal2002 at gmail.com>
>> ---
>> button.py | 10 ++--
>> model.py | 2 +
>> p5_button.py | 30 +++++-----
>> ui.py | 199 +++++++++++++++++++++++++++++++---------------------------
>> 4 files changed, 129 insertions(+), 112 deletions(-)
>>
>> diff --git a/button.py b/button.py
>> index 14b9700..2b0e85c 100644
>> --- a/button.py
>> +++ b/button.py
>> @@ -62,12 +62,12 @@ class RecdButton(TrayButton, gobject.GObject):
>> return img
>>
>>
>> - def setButtClickedId( self, id ):
>> - self.BUTT_CLICKED_ID = id
>> + def setButtonClickedId( self, id ):
>> + self.BUTTON_CLICKED_ID = id
>>
>>
>> - def getButtClickedId( self ):
>> - return self.BUTT_CLICKED_ID
>> + def getButtonClickedId( self ):
>> + return self.BUTTON_CLICKED_ID
>>
>>
>> def setup_rollover_options( self, info ):
>> @@ -105,4 +105,4 @@ class RecdButton(TrayButton, gobject.GObject):
>>
>>
>> def _itemCopyToClipboardCb(self, widget):
>> - self.ui.copyToClipboard( self.recd )
>> \ No newline at end of file
>> + self.ui.copyToClipboard( self.recd )
>> diff --git a/model.py b/model.py
>> index b7f592b..e24752e 100644
>> --- a/model.py
>> +++ b/model.py
>> @@ -323,6 +323,8 @@ class Model:
>> def startTakingPhoto( self ):
>> self.setUpdating( True )
>> self.ca.glive.takePhoto()
>> + self.ca.ui.FULLSCREEN = False
>> + self.ca.ui.updateVideoComponents()
>
> This needs some explanation.
> We can be taking photos in full screen mode.
> There are also no changes in the video components when we are taking a photo.
>
> So these 2 lines seem wrong/unnecessary or indicative of a larger problem.
>
>
>> @@ -335,7 +335,7 @@ class UI:
>> self.tagsBuffer = gtk.TextBuffer()
>> self.tagsBuffer.connect('changed', self._tagsBufferEditedCb)
>> self.tagsField = gtk.TextView(self.tagsBuffer)
>> - self.tagsField.set_size_request( 100, 100 )
>> + self.tagsField.set_size_request( 50, 50 )
>> self.tagsPanel.pack_start(self.tagsField, expand=True)
>> self.infoBoxTopLeft.pack_start(self.tagsPanel, expand=True)
>
> Why this change?
>
>> @@ -369,7 +369,6 @@ class UI:
>> self.centered = True
>> self.setUp()
>>
>> -
>> def _mapEventCb( self, widget, event ):
>> #when your parent window is ready, turn on the feed of live video
>> self.liveVideoWindow.disconnect(self.MAP_EVENT_ID)
>> @@ -461,9 +460,11 @@ class UI:
>> self.slowLiveVideoWindow.connect("visibility-notify-event", self._visibleNotifyCb)
>>
>> self.recordWindow = RecordWindow(self)
>> + self.recordWindow.set_geometry_hints(min_width=1, min_height=1)
>> self.addToWindowStack( self.recordWindow, self.windowStack[len(self.windowStack)-1] )
>>
>> self.progressWindow = ProgressWindow(self)
>> + self.progressWindow.set_geometry_hints(min_width=1, min_height=1)
>> self.addToWindowStack( self.progressWindow, self.windowStack[len(self.windowStack)-1] )
>>
>> self.maxWindow = gtk.Window()
>> @@ -471,9 +472,11 @@ class UI:
>> self.maxWindow.modify_bg( gtk.STATE_INSENSITIVE, Constants.colorBlack.gColor )
>> maxButton = MaxButton(self)
>> self.maxWindow.add( maxButton )
>> + self.maxWindow.set_geometry_hints(min_width=1, min_height=1)
>> self.addToWindowStack( self.maxWindow, self.windowStack[len(self.windowStack)-1] )
>>
>> self.scrubWindow = ScrubberWindow(self)
>> + self.scrubWindow.set_geometry_hints(min_width=1, min_height=1)
>> self.addToWindowStack( self.scrubWindow, self.windowStack[len(self.windowStack)-1] )
>>
>> self.infWindow = gtk.Window()
>> @@ -492,6 +495,7 @@ class UI:
>>
>>
>> def _visibleNotifyCb( self, widget, event ):
>> + logging.debug("_visibleNotifyCb")
>>
>> if (self.LAUNCHING):
>> return
>> @@ -525,6 +529,31 @@ class UI:
>> else:
>> self.ca.stopPipes()
>>
>> + def resizeWindows( self ):
>> + if self.ca.m.MODE == Constants.MODE_AUDIO:
>> + self.maxWindow.resize( 1, 1 )
>> + else:
>> + maxDim = self.getMaxDim( self.FULLSCREEN )
>> + self.maxWindow.resize( maxDim[0], maxDim[1] )
>> +
>> + if self.LIVEMODE:
>> + self.pipBgdWindow.resize( 1, 1 )
>> + self.playOggWindow.resize( 1, 1 )
>> + self.livePhotoWindow.resize( 1, 1 )
>> + self.slowLiveVideoWindow.resize( 1, 1 )
>> + self.infWindow.resize( 1, 1 )
>> + if self.ca.m.MODE == Constants.MODE_PHOTO:
>> + self.progressWindow.resize( 1, 1 )
>> + else:
>> + pgdDim = self.getPgdDim( self.FULLSCREEN )
>> + self.pipBgdWindow.resize( pgdDim[0], pgdDim[1] )
>> + self.progressWindow.resize( 1, 1 )
>> + self.recordWindow.resize( 1, 1 )
>> + if self.ca.m.MODE == Constants.MODE_PHOTO:
>> + self.playOggWindow.resize( 1, 1 )
>> + self.scrubWindow.resize( 1, 1 )
>> + elif self.ca.m.MODE == Constants.MODE_VIDEO:
>> + self.liveVideoWindow.resize( 1, 1 )
>>
>> def setUpWindowsSizes( self ):
>> pipDim = self.getPipDim(False)
>> @@ -597,30 +626,6 @@ class UI:
>> if (self.HIDE_WIDGET_TIMEOUT_ID != 0):
>> gobject.source_remove( self.HIDE_WIDGET_TIMEOUT_ID )
>>
>> -
>> - def hideWidgets( self ):
>> - self.moveWinOffscreen( self.maxWindow )
>> - self.moveWinOffscreen( self.pipBgdWindow )
>> - self.moveWinOffscreen( self.infWindow )
>> - self.moveWinOffscreen( self.slowLiveVideoWindow )
>> -
>> - if (self.FULLSCREEN):
>> - self.moveWinOffscreen( self.recordWindow )
>> - self.moveWinOffscreen( self.progressWindow )
>> - self.moveWinOffscreen( self.scrubWindow )
>> -
>> - if (self.ca.m.MODE == Constants.MODE_PHOTO):
>> - if (not self.LIVEMODE):
>> - self.moveWinOffscreen( self.liveVideoWindow )
>> - elif (self.ca.m.MODE == Constants.MODE_VIDEO):
>> - if (not self.LIVEMODE):
>> - self.moveWinOffscreen( self.liveVideoWindow )
>> - elif (self.ca.m.MODE == Constants.MODE_AUDIO):
>> - if (not self.LIVEMODE):
>> - self.moveWinOffscreen( self.liveVideoWindow )
>> - self.LAST_MODE = -1
>> -
>> -
>> def _mouseMightaMovedCb( self ):
>> x, y = self.ca.get_pointer()
>> passedTime = 0
>> @@ -637,18 +642,6 @@ class UI:
>> self.hideWidgetsTimer = time.time()
>> passedTime = 0
>>
>> - if (passedTime >= 3):
>> - if (not self.hiddenWidgets):
>> - if (self.mouseInWidget(x,y)):
>> - self.hideWidgetsTimer = time.time()
>> - elif (self.RECD_INFO_ON):
>> - self.hideWidgetsTimer = time.time()
>> - elif (self.UPDATE_TIMER_ID != 0):
>> - self.hideWidgetsTimer = time.time()
>> - else:
>> - self.hideWidgets()
>> - self.hiddenWidgets = True
>> -
>> self.mx = x
>> self.my = y
>> return True
>> @@ -680,6 +673,7 @@ class UI:
>>
>>
>> def _mediaClickedForPlayback(self, widget, event):
>> + logging.debug("_mediaClickedForPlayback")
>> if (not self.LIVEMODE):
>> if (self.shownRecd != None):
>> if (self.ca.m.MODE != Constants.MODE_PHOTO):
>> @@ -875,18 +869,18 @@ class UI:
>> kids = self.thumbTray.get_children()
>> for i in range (0, len(kids)):
>> if (self.ca.m.UPDATING or self.ca.m.RECORDING):
>> - if (kids[i].getButtClickedId() != 0):
>> - kids[i].disconnect( kids[i].getButtClickedId() )
>> - kids[i].setButtClickedId(0)
>> + if (kids[i].getButtonClickedId() != 0):
>> + kids[i].disconnect( kids[i].getButtonClickedId() )
>> + kids[i].setButtonClickedId(0)
>> else:
>> - if (kids[i].getButtClickedId() == 0):
>> - BUTT_CLICKED_ID = kids[i].connect( "clicked", self._thumbClicked, kids[i].recd )
>> - kids[i].setButtClickedId(BUTT_CLICKED_ID)
>> + if (kids[i].getButtonClickedId() == 0):
>> + BUTTON_CLICKED_ID = kids[i].connect( "clicked", self._thumbClicked, kids[i].recd )
>> + kids[i].setButtonClickedId(BUTTON_CLICKED_ID)
>>
>>
>> def hideAllWindows( self ):
>> for i in range (0, len(self.windowStack)):
>> - self.moveWinOffscreen( self.windowStack[i] )
>> + self.windowStack[i].hide_all()
>>
>>
>> def _liveButtonReleaseCb(self, widget, event):
>> @@ -1003,6 +997,7 @@ class UI:
>> self.doMouseListener( True )
>> self.showLiveVideoTags()
>> self.LAST_MODE = -1 #force an update
>> +
>> self.updateVideoComponents()
>> self.resetWidgetFadeTimer()
>>
>> @@ -1010,7 +1005,8 @@ class UI:
>> def startLiveVideo(self, force):
>> #We need to know which window and which pipe here
>>
>> - #if returning from another activity, active won't be false and needs to be to get started
>> + #if returning from another activity, active won't be false
>> + #and needs to be to get started
>> if (self.ca.glive.window == self.liveVideoWindow
>> and self.ca.props.active
>> and not force):
>> @@ -1025,14 +1021,6 @@ class UI:
>> self.FULLSCREEN = not self.FULLSCREEN
>> self.updateVideoComponents()
>>
>> -
>> - def moveWinOffscreen( self, win ):
>> - #we move offscreen to resize or else we get flashes on screen, and setting hide() doesn't allow resize & moves
>> - offW = (gtk.gdk.screen_width() + 100)
>> - offH = (gtk.gdk.screen_height() + 100)
>> - self.smartMove(win, offW, offH)
>> -
>> -
>> def setImgLocDim( self, win ):
>> imgDim = self.getImgDim( self.FULLSCREEN )
>> self.smartResize( win, imgDim[0], imgDim[1] )
>> @@ -1175,12 +1163,12 @@ class UI:
>> if (self.ca.m.MODE == Constants.MODE_PHOTO):
>> return [self.vw, self.controlBarHt]
>> else:
>> - return [self.recordButtWd, self.controlBarHt]
>> + return [self.recordButtonWd, self.controlBarHt]
>> else:
>> if (self.ca.m.MODE == Constants.MODE_PHOTO):
>> return [gtk.gdk.screen_width()-(self.inset*2), self.controlBarHt]
>> else:
>> - return [self.recordButtWd, self.controlBarHt]
>> + return [self.recordButtonWd, self.controlBarHt]
>>
>>
>> def getInbLoc( self, full ):
>> @@ -1253,16 +1241,16 @@ class UI:
>>
>> def getPrgDim( self, full ):
>> if (not full):
>> - return [self.vw-self.recordButtWd, self.controlBarHt]
>> + return [self.vw-self.recordButtonWd, self.controlBarHt]
>> else:
>> - return [gtk.gdk.screen_width()-(self.inset+self.inset+self.recordButtWd), self.controlBarHt]
>> + return [gtk.gdk.screen_width()-(self.inset+self.inset+self.recordButtonWd), self.controlBarHt]
>>
>>
>> def getPrgLoc( self, full ):
>> if (not full):
>> - return [self.centerBoxPos[0]+self.recordButtWd, self.centerBoxPos[1]+self.vh]
>> + return [self.centerBoxPos[0]+self.recordButtonWd, self.centerBoxPos[1]+self.vh]
>> else:
>> - return [self.inset+self.recordButtWd, gtk.gdk.screen_height()-(self.inset+self.controlBarHt)]
>> + return [self.inset+self.recordButtonWd, gtk.gdk.screen_height()-(self.inset+self.controlBarHt)]
>>
>>
>> def getLoc( self, pos, full ):
>> @@ -1316,6 +1304,7 @@ class UI:
>> else:
>> #or, if there is no countdown, it might be because we are recording
>> self.clickShutter()
>> + self.progressWindow.updateProgress( 1, Constants.istrFinishedRecording )
>
> This change is also surprising to see.
>
> Daniel
>
More information about the Sugar-devel
mailing list