> On Jan. 12, 2014, 9:57 p.m., Albert Astals Cid wrote: > > shell/shell.cpp, line 181 > > <https://git.reviewboard.kde.org/r/110914/diff/9/?file=233820#file233820line181> > > > > This check seems a bit weird, i don't even think you need it, but if > > you want one check there shouldn't you be doing something like > > activeTab < m_tabs.size? > > > > But this can't happen, no? > > Jonathan Doman wrote: > Yes, you are right. I was specifically trying to change the code as > little as possible, and there was previously a check for m_part which was > also weird and unnecessary. > > Albert Astals Cid wrote: > Well, the check was checking that "the part that we were going to use was > really there", since there was just one part, it only checked for the > pointer, now you are checking that there is any part and then you access one > that may not be there, please change it so it checks for < m_tabs.size
(Well, I don't think you could ever get in that function if m_part wasn't there.) Anyway, fixed. > On Jan. 12, 2014, 9:57 p.m., Albert Astals Cid wrote: > > shell/shell.cpp, line 206 > > <https://git.reviewboard.kde.org/r/110914/diff/9/?file=233820#file233820line206> > > > > should this be m_tabs[activeTab] instead of m_tabs[0]? > > Jonathan Doman wrote: > Maybe. In that section of code it is guaranteed that activeTab==0, since > there will never be an empty part when there are other tabs open. I was going > to put an assertion in there to make that clear, but I noticed there are no > assertions anywhere in the code and thought it might be some kind of policy. > > Albert Astals Cid wrote: > You sure about that? if a document contains a DocumentAction::Close: > action the part will close the document, but the part won't be destroyed, no? > Hmmm, or maybe it will actually will. Anyway if it is guaranteed that > activeTab is 0, i think it's easier to undertand the code if you actually use > activeTab there since it flows better with reading the code. If you want to > add an assert that's fine (Q_ASSERT), but also make sure the code does not > crash even if the assert is not met for people that run with Q_ASSERT > disabled. If there is a way to close the document without going through the shell, then I suppose it is a problem. > On Jan. 12, 2014, 9:57 p.m., Albert Astals Cid wrote: > > shell/shell.cpp, line 256 > > <https://git.reviewboard.kde.org/r/110914/diff/9/?file=233820#file233820line256> > > > > space here > > Jonathan Doman wrote: > Are you sure? Most config keys (in this program and others) are camel > cased. > > Albert Astals Cid wrote: > Sorry, was unclear, just meant after the comma. Oh, the spacing was intentional. Anyway, fixed. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/110914/#review47271 ----------------------------------------------------------- On Jan. 13, 2014, 12:24 a.m., Jonathan Doman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/110914/ > ----------------------------------------------------------- > > (Updated Jan. 13, 2014, 12:24 a.m.) > > > Review request for Okular. > > > Bugs: 155515 > http://bugs.kde.org/show_bug.cgi?id=155515 > > > Repository: okular > > > Description > ------- > > This patch adds support for a tabbed interface (multiple documents in one > window). The core work just adds a tab bar that switches between multiple > embedded okularparts, but there are many other considerations: > - Tab context menu allows for duplicating or detaching (detached tabs start > in new okular process) > - `okular file.pdf` will open file in existing window if possible, unless > --new flag is used. It also selects the most recently raised/activated window > to use. This mirrors behavior I expect from browsers and other tabbed > interfaces. > - Warns when closing window with multiple tabs > - No warning is given when opening an already open file. This is the > behavior I strongly prefer (and observe in other programs), but will change > if there is consensus otherwise. > > When selecting different tools in one part, the tool selection propagates to > all parts, but the GUI does not reflect that. This bug is present in other > programs (e.g. multiple okularparts in Konqueror), so I made no attempt to > diagnose or fix. > > One menu item was added for the multiple tab warning option. When testing > this, I noticed that items in the Settings menu seem to move around when > switching tabs, and I cannot diagnose or fix this. It seems to be related to > XMLGUI bug #64754. > > My development branch is also hosted at > https://github.com/jrmrjnck/okular-tabbed > > > Diffs > ----- > > part.h 4b3aafdb637080ae81eb0e45742f53a34738984d > part.cpp 88e2c41059344dbd11d4c94f99c63a3f5bc8c99b > shell/shell.h c065c560fb4ddfcf181601cf35e9ca14581731f6 > shell/shell.cpp 1708501daaef817a1ce35fa5d96701a66ab66983 > shell/shell.rc 93fbc417588312792bab39b693c65e5d414c87c6 > > Diff: https://git.reviewboard.kde.org/r/110914/diff/ > > > Testing > ------- > > > Thanks, > > Jonathan Doman > >
_______________________________________________ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
