D11051: Remembering side navigation panel state

[Nathaniel Graham]

ngraham abandoned this revision.
ngraham added a comment.


  This is on GitLab now: https://invent.kde.org/kde/okular/merge_requests/22

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: ngraham, #okular, aacid, dileepsankhla
Cc: okular-devel, tobiasdeiminger, aacid, sander, #okular, andisa, 
siddharthmanthan, maguirre, fbampaloukas, joaonetto, kezik, tfella, ngraham, 
darcyshen

D11051: Remembering side navigation panel state

[Nathaniel Graham]

ngraham commandeered this revision.
ngraham added a reviewer: dileepsankhla.
Herald added a subscriber: okular-devel.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: ngraham, #okular, aacid, dileepsankhla
Cc: okular-devel, tobiasdeiminger, aacid, sander, #okular, andisa, 
siddharthmanthan, maguirre, fbampaloukas, joaonetto, kezik, tfella, ngraham, 
darcyshen

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla added a comment.


  Currently I am working as a GSoC student and in discussion with my mentor, I 
have decided to pause the patch report during the GSoC period. BBL

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: tobiasdeiminger, aacid, sander, #okular, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: tobiasdeiminger, aacid, sander, #okular, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  In D11051#239497 , @dileepsankhla 
wrote:
  
  > The state can be saved whenever a sidebar item is clicked in 
Sidebar::itemClicked but again as discussed earlier, it will be asymmetrical as 
to save in sidebar.cpp. Should I implement a signal slot mechanism in part.cpp 
to achieve so or should I find another way?
  
  
  Ok, let's try to move the code to Sidebar again if you think there is better.
  
  Also would be very nice if you could add autotests for this (and the sidebar 
visibility feature that is a sister feature) to make sure it doesn't break.
  
  Basically,
  open part, show sidebar, close part, open part, check sidebar is visible, 
hide part, open part, check sidebar is not visible
  open part, show sidebar, collapse sidebar, close part, open part, check 
sidebar is visible but collapsed
  and the same variants also opening files, specially files with toc, since the 
also had the problem that was uncollapsing the sidebar when opening files for 
no reason.

INLINE COMMENTS

> part.cpp:439
> +
> +// Setting current item as thumbsBox if the side navigation panel is 
> visible 
> +if ( !Okular::Settings::hideSideContainer() )

can you please change your nomenclarute of "side navigation panel is visible" 
in this and the other comments, because that is what F7 does, which is sabed 
with m_sidebar->setSidebarVisibility and what you're saving is the collapsed 
status.

Also the config option would probably make more sense to be named something 
like sidebarCollapsed defaulting to false.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: tobiasdeiminger, aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla added a comment.


  The state can be saved whenever a sidebar item is clicked in 
Sidebar::itemClicked but again as discussed earlier, it will be asymmetrical as 
to save in sidebar.cpp. Should I implement a signal slot mechanism in part.cpp 
to achieve so or should I find another way?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: tobiasdeiminger, aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  In D11051#239040 , 
@tobiasdeiminger wrote:
  
  > > One thing that may fix it is changing
  > > 
  > >   m_sidebar = new Sidebar( parentWidget );
  > > 
  > > to
  > > 
  > >   m_sidebar = new Sidebar();
  > > 
  > > in part.cpp, this micht change the part/child relationships and get 
yourself of this problem. can you give it a quick try?
  >
  > Seems like sidebar becomes a (grand-)child of Shells QTabWidget, no matter 
what you set as parent widget in Sidebar Ctor. This is because of ##setWidget( 
m_sidebar )## in Part::Part(), and  ##m_tabWidget->addTab( firstPart->widget(), 
QString() )## in Shell::Shell(). Then C++ Dtor order lets Shell::~Shell (cleans 
up child widgets, including sidebar) be called earlier than Part::~Part.
  >
  > > Should we stick to it or should we consider saving the stats in the 
Sidebar::~Sidebar()?
  >
  > Would it be an option to save sidebar state in ##Part::closeUrl()##?
  >  It is called early enough by Shell Dtor. For me it fixes the segfault, and 
"closeUrl" sounds like a reasonable action to cause document related state 
saving.
  
  
  The thing is that whether you have the panel open or not is not really 
document related.
  
  I think what we should do is just save the option when it changes, like we do 
in Part::slotShowLeftPanel and Part::slotShowBottomBar

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: tobiasdeiminger, aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  In D11051#238586 , @dileepsankhla 
wrote:
  
  > I tried but it is still giving me the segfault. The interesting thing, in 
my opinion, is whenever you open Okular and close it down without changing the 
sidebar state, it doesn't give a segfault whereas changing the sidebar state 
and closing it gives the error. 
  >  Should we stick to it
  
  
  Stick to what?
  
  > or should we consider saving the stats in the Sidebar::~Sidebar()?
  
  I already said that saving the value in sidebar and loading it in part is 
very un-intuitive, that doesn't seem to have changed ;)

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: tobiasdeiminger, aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Tobias Deiminger]

tobiasdeiminger added a comment.


  > One thing that may fix it is changing
  > 
  >   m_sidebar = new Sidebar( parentWidget );
  > 
  > to
  > 
  >   m_sidebar = new Sidebar();
  > 
  > in part.cpp, this micht change the part/child relationships and get 
yourself of this problem. can you give it a quick try?
  
  Seems like sidebar becomes a (grand-)child of Shells QTabWidget, no matter 
what you set as parent widget in Sidebar Ctor. This is because of ##setWidget( 
m_sidebar )## in Part::Part(), and  ##m_tabWidget->addTab( firstPart->widget(), 
QString() )## in Shell::Shell(). Then C++ Dtor order lets Shell::~Shell (cleans 
up child widgets, including sidebar) be called earlier than Part::~Part.
  
  > Should we stick to it or should we consider saving the stats in the 
Sidebar::~Sidebar()?
  
  Would it be an option to save sidebar state in ##Part::closeUrl()##?
  It is called early enough by Shell Dtor. For me it fixes the segfault, and 
"closeUrl" sounds like a reasonable action to cause document related state 
saving.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: tobiasdeiminger, aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla added a comment.


  I tried but it is still giving me the segfault. The interesting thing, in my 
opinion, is whenever you open Okular and close it down without changing the 
sidebar state, it doesn't give a segfault whereas changing the sidebar state 
and closing it gives the error. 
  Should we stick to it or should we consider saving the stats in the 
Sidebar::~Sidebar()?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  So https://paste.kde.org/pwvknplv2 shows the problem
  
  Shell is deleting m_tabWidget which is deleting the sidebars before the parts 
are deleted.
  
  Do you understand that from reading that piece of text?
  
  One thing that may fix it is changing
  
m_sidebar = new Sidebar( parentWidget );
  
  to
  
m_sidebar = new Sidebar();
  
  in part.cpp, this micht change the part/child relationships and get yourself 
of this problem. can you give it a quick try?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla added a comment.


  The verbose output of valgrind is around 190,000 lines with the line numbers. 
Here is the output:
  
  https://dileepsankhla.com/valgrind.txt

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  This is the important part https://paste.kde.org/pldoxclam
  
  It tells you that you're trying to use Sidebar while it was already deleted. 
Problem is you did not compile with debug enabled and the log is not as useful 
as it could. Please add -DCMAKE_BUILD_TYPE=Debug to your cmake line and build 
again and you'll get line numbers which are much nicer. Paste the valgrind log 
again once you do that.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla added a comment.


  https://paste.kde.org/pepnfuzxq 

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  In D11051#237655 , @dileepsankhla 
wrote:
  
  > Following is the log of running valgrind on okular:
  >
  > https://paste.kde.org/pmyjdscmw/ohzvag 

  
  
  This paste is password protected, why did you do that?
  
  Can you please paste it again without a password?
  
  > I have no luck in understanding the verbose. May you please help me by 
checking it out?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla added a comment.


  Following is the log of running valgrind on okular:
  
  https://paste.kde.org/pmyjdscmw/ohzvag 

  
  I have no luck in understanding the verbose. May you please help me by 
checking it out?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  In D11051#235502 , @dileepsankhla 
wrote:
  
  > Actually I am getting the segmentation fault whenever I close Okular with a 
changed state of the side navigation panel and the code causing it is 
`Okular::Settings::setHideSideContainer( m_sidebar->isCollapsed() )` inside the 
Part destructor. `m_sidebar` is not causing this problem as I misunderstood the 
earlier; it gives the segfault for any value as the argument of 
`setHideSideContainer` except for the boolean values `True` and `False`.  It 
works fine for the absolute boolean values.
  >  Running it with gdb, I got segfault in 
`QAbstractScrollArea::horizontalScrollBarPolicy() const () from 
/usr/lib/libQt5Widgets.so.5` that is confusing for me. I need an idea regarding 
what can be done in this case?
  
  
  run the program in valgrind, it usually gives you a nicer log of where the 
problem with memory handling is happening, i'm curious why 
horizontalScrollBarPolicy may be the problem.
  
  If you have trouble understanding the valgrind output, please post it here 
and I'll try to help reading it.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla updated this revision to Diff 30742.
dileepsankhla added a comment.


  Updating: Remembring side navigation panel state
  Changing the state to the one being discussed here in comments.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11051?vs=28694=30742

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D11051

AFFECTED FILES
  conf/okular.kcfg
  part.cpp

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla added a comment.


  Actually I am getting the segmentation fault whenever I close Okular with a 
changed state of the side navigation panel and the code causing it is 
`Okular::Settings::setHideSideContainer( m_sidebar->isCollapsed() )` inside the 
Part destructor. `m_sidebar` is not causing this problem as I misunderstood the 
earlier; it gives the segfault for any value as the argument of 
`setHideSideContainer` except for the boolean values `True` and `False`.  It 
works fine for the absolute boolean values.
  Running it with gdb, I got segfault in 
`QAbstractScrollArea::horizontalScrollBarPolicy() const () from 
/usr/lib/libQt5Widgets.so.5` that is confusing for me. I need an idea regarding 
what can be done in this case?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular, aacid
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  In D11051#222803 , @dileepsankhla 
wrote:
  
  > In D11051#05 , @aacid wrote:
  >
  > > Also i'm not convinced it's a good idea to save the settings in 
sidebar.cpp but read them in part.cpp, seems a bit asymmetrical, can you 
explain your reasoning for it?
  >
  >
  > Initially, I thought of reading and saving the settings in `part.cpp` but I 
can't access `m_sidebar` as to save its last state inside the destructor 
`Part::~Part` and it gives me segfault. Hence I am saving them in 
`Sidebar::~Sidebar`.
  
  
  I just added 
  qDebug() << m_sidebar->isCollapsed();
  to Part destructor and it's all fine, i don't see why you would get a 
segfault, and if you did, you should investigate why, if you thought that Part 
destructor was a better place instead of just saying "meh i've no idea why it 
happens, let's put it in a worse place"

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla marked an inline comment as done.
dileepsankhla added a comment.


  In D11051#05 , @aacid wrote:
  
  > Also i'm not convinced it's a good idea to save the settings in sidebar.cpp 
but read them in part.cpp, seems a bit asymmetrical, can you explain your 
reasoning for it?
  
  
  Initially, I thought of reading and saving the settings in `part.cpp` but I 
can't access `m_sidebar` as to save its last state inside the destructor 
`Part::~Part` and it gives me segfault. Hence I am saving them in 
`Sidebar::~Sidebar`.
  
  > Do you think you can give it a try at adding an auto test for this?
  
  Sure, let me know your thoughts on where to save the settings and then I will 
try to write the auto test for this.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Albert Astals Cid]

aacid added a comment.


  Do you think you can give it a try at adding an auto test for this?
  
  Also i'm not convinced it's a good idea to save the settings in sidebar.cpp 
but read them in part.cpp, seems a bit asymmetrical, can you explain your 
reasoning for it?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular
Cc: aacid, sander, #okular, michaelweghorn, ngraham

D11051: Remembering side navigation panel state

[Oliver Sander]

sander added a comment.


  I tried the patch and it does what it claims to do.  I have no further 
objections.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular
Cc: sander, #okular, michaelweghorn, ngraham, aacid

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla updated this revision to Diff 28694.
dileepsankhla added a comment.


  Updating: Remembering side navigation panel state
  Removed the extra whitespace.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11051?vs=28693=28694

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D11051

AFFECTED FILES
  conf/okular.kcfg
  part.cpp
  ui/sidebar.cpp

To: dileepsankhla, #okular
Cc: sander, #okular, michaelweghorn, ngraham, aacid

D11051: Remembering side navigation panel state

[Oliver Sander]

sander added inline comments.

INLINE COMMENTS

> sidebar.cpp:717
>  {
> +
>  if ( !item )

Please avoid such white-space changes.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular
Cc: sander, #okular, michaelweghorn, ngraham, aacid

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla added a reviewer: Okular.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11051

To: dileepsankhla, #okular
Cc: #okular, michaelweghorn, ngraham, aacid

D11051: Remembering side navigation panel state

[Dileep Sankhla]

dileepsankhla created this revision.
Restricted Application added a subscriber: Okular.
Restricted Application added a project: Okular.
dileepsankhla requested review of this revision.

REVISION SUMMARY
  Every time Okular starts, one of the items of the side navigation panel is 
open. There is no way to disable it globally. It would be convenient to the 
users to automatically save the state of the panel upon exit and restore it 
when the Okular is opened again.
  FEATURE: 344599

TEST PLAN
  1. Open a PDF with Okular
  2. You will find that one of the items of the side navigation panel is open
  3. Click on the item icon of the navigation panel to minimize item's panel
  4. Close Okular
  5. Reopen Okular with same or different file as in step 1
  6. One of the items of the side navigation panel is again open
  7. But after applying this patch, the side navigation panel should be remain 
collapsed in step 6 with none of the item is selected

REPOSITORY
  R223 Okular

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D11051

AFFECTED FILES
  conf/okular.kcfg
  part.cpp
  ui/sidebar.cpp

To: dileepsankhla
Cc: #okular, michaelweghorn, ngraham, aacid