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

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?

In D11051#222205, @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.

In D11051#222803, @dileepsankhla wrote:

In D11051#222205, @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"

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?

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.

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?

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?

https://paste.kde.org/pepnfuzxq

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.

The verbose output of valgrind is around 190,000 lines with the line numbers. Here is the output:

https://dileepsankhla.com/valgrind.txt

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?

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()?

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.

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 ;)

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

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?

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.

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