> On ago. 10, 2015, 10:59 p.m., Albert Astals Cid wrote: > > How does this code play with the other code that remembers this stuff > > globally? Should that code be removed? > > Felix Mauch wrote: > yes, revision 2 reverts those changes, so revision 1 and 2 give the > complete change. Is this the wrong way to post it? I'm sorry, I'm working > with the reviewboard for the first time. Should I post complete diffs from > the master instead so the different revisions will be independent?
You always put a complete diff against master. > On ago. 10, 2015, 10:59 p.m., Albert Astals Cid wrote: > > ui/pageview.cpp, line 1402 > > <https://git.reviewboard.kde.org/r/123427/diff/2/?file=390795#file390795line1402> > > > > You should use a string here and not an int, otherwise if in the future > > we add, move or remove items in this menu the numbers are going to be a > > pain to store/restore > > Felix Mauch wrote: > I don't like hard-coded strings very much, as you'll have to change all > those later on, as well. However, I agree, that my simple solution is even > worse than that. What about using the data field of the action? The viewMode > enum-Type from Okular::Settings is stored in there. So when new modes are > added there or the menu entries are reordered, no further changes should be > necessary. However, I'd still have to use an int internally, as I can't save > the enum type in the QVariant. Yet, I could to a casting the > > ```cpp > int mode; > for (int i=0; i < d->aViewMode->menu()->actions().size(); ++i) { > const QAction* action = d->aViewMode->menu()->actions().at(i); > if(action->isChecked() ) { > QVariant mode_id = action->data(); > mode = mode_id.toInt(); > break; > } > } > return mode; > ``` Yes using something like the viewMode that doesn't tie physical order of the actions to the saved setting is much better. - Albert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123427/#review83684 ----------------------------------------------------------- On ago. 6, 2015, 3:22 p.m., Felix Mauch wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/123427/ > ----------------------------------------------------------- > > (Updated ago. 6, 2015, 3:22 p.m.) > > > Review request for Okular. > > > Bugs: 341318 > http://bugs.kde.org/show_bug.cgi?id=341318 > > > Repository: okular > > > Description > ------- > > Adds the functiionality to save the view mode (single page, facing...) and > continuous scrolling to the document information as it is already done with > the zoom information. > > > Diffs > ----- > > core/document.cpp 9d12488 > core/view.h a369d24 > ui/pageview.h e65b575 > ui/pageview.cpp b57e6ae > > Diff: https://git.reviewboard.kde.org/r/123427/diff/ > > > Testing > ------- > > I use a version with this patch applied since a couple of days. The intended > functionality seems to work reliably. > Maybe it would be nice to implement a "standard view" selection in the > options dialog as it's done for the zoom mode. > > > Thanks, > > Felix Mauch > >
_______________________________________________ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
