#2838 HIGH Opportu: Read should save in the journal state info

[Zarro Boogs per Child]

#2838: Read should save in the journal state info
----------------------------+-----------------------------------------------
  Reporter:  tomeu          |       Owner:  tomeu                     
      Type:  enhancement    |      Status:  new                       
  Priority:  high           |   Milestone:  Opportunity (please help!)
 Component:  read-activity  |     Version:                            
Resolution:                 |    Keywords:  review?                   
  Verified:  0              |  
----------------------------+-----------------------------------------------

Comment(by tomeu):

 {{{
 +        # Save the zoom level /before/ the sizing_mode, because during
 page load
 +        # we assume 'Read_zoom' exists when 'Read_sizing_mode' exists.
 }}}

 No need to take this care when saving, as at load time we shouldn't never
 rely on one property existing (or having a specific format). See below.

 {{{
 +        # pascals: This may not be the best way to save an enum value,
 but this works...
 +        if self._view.props.sizing_mode == evince.SIZING_BEST_FIT:
 +            self.metadata['Read_sizing_mode'] = "best-fit"
 +        elif self._view.props.sizing_mode == evince.SIZING_FREE:
 +            self.metadata['Read_sizing_mode'] = "free"
 +        elif self._view.props.sizing_mode == evince.SIZING_FIT_WIDTH:
 +            self.metadata['Read_sizing_mode'] = "fit-width"
 +        else:
 +            self.metadata['Read_sizing_mode'] = ""
 }}}

 Looks good to me, but I would log a warning in the else.

 {{{
 +        if self.metadata['Read_current_page']:
 +
 self._document.get_page_cache().set_current_page(int(self.metadata['Read_current_page']))
 }}}

 What about self.metadata.get('Read_current_page', '0') ? Same for the
 other properties.

 {{{
 +            # It would be nice if we could highlight the search term
 again, but this causes a
 +            # jump to the next find, which may not be on the current
 page. Something like:
 +            #if self._edit_toolbar._search_entry.props.text != "":
 +            #
 self._document.find_begin(self._document.get_page_cache().get_current_page(),
 \
 +            #
 self._edit_toolbar._search_entry.props.text, False)
 }}}

 As an alternative, we could do first the search and afterwards position in
 Read_current_page. Looks good?

 The patch is very good. Could you please also check that lines don't
 extend over 80 chars, more or less?

 I'll push it when you attach the updated patch.

 Thanks!

-- 
Ticket URL: <https://dev.laptop.org/ticket/2838#comment:6>
One Laptop Per Child <https://dev.laptop.org>
OLPC bug tracking system