Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make grid options and two others available to the embedded pdf-viewer #3942

Merged
merged 6 commits into from
Jan 13, 2025
Merged

make grid options and two others available to the embedded pdf-viewer #3942

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7f83e03
add toolbutton grid actions to embedded pdf-viewer
octaeder Dec 31, 2024
043ec94
add Grid menu to context menu, no toolbar button
octaeder Jan 10, 2025
1a65c06
removing unnecessary changes
octaeder Jan 11, 2025
59a4ca1
use partly seperate config for embedded viewer
octaeder Jan 12, 2025
b971f45
further simplify if conditions
octaeder Jan 12, 2025
6ad8a1a
factor commen string
octaeder Jan 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 92 additions & 31 deletions src/pdfviewer/PDFDocument.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,7 +896,7 @@ void PDFWidget::setPDFDocument(PDFDocument *docu)
pdfdocument = docu;
}

void PDFWidget::setDocument(const QSharedPointer<Poppler::Document> &doc)
void PDFWidget::setDocument(const QSharedPointer<Poppler::Document> &doc, bool embedded)
{
pages.clear();
document = doc;
Expand All @@ -906,7 +906,10 @@ void PDFWidget::setDocument(const QSharedPointer<Poppler::Document> &doc)

if (!document.isNull()) {
docPages = document->numPages();
setSinglePageStep(globalConfig->singlepagestep);
if (embedded)
setSinglePageStep(false);
else
setSinglePageStep(globalConfig->singlepagestep);
} else
docPages = 0;
#ifdef MEDIAPLAYER
Expand Down Expand Up @@ -1586,9 +1589,16 @@ void PDFWidget::contextMenuEvent(QContextMenuEvent *event)
usingTool = kNone;
}

if (pdfDoc && pdfDoc->menuShow) {
menu.addSeparator();
menu.addMenu(pdfDoc->menuShow);
if (pdfDoc) {
if (pdfDoc->menuGrid || pdfDoc->menuShow) {
menu.addSeparator();
if (pdfDoc->menuGrid) {
menu.addMenu(pdfDoc->menuGrid);
}
if (pdfDoc->menuShow) {
menu.addMenu(pdfDoc->menuShow);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overly complicated code
apart from the fact the the pointers are not zeroed in the first place, there is no case when only one of the two appears.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplified and assinged nullptr in headerfile

}

QAction *action = menu.exec(event->globalPos());
Expand Down Expand Up @@ -2098,10 +2108,14 @@ void PDFWidget::setPageOffset(int offset, bool setAsDefault, bool refresh){
pageOffset = offset;
else {
pageOffset = gridx - 1;
globalConfig->pageOffset = pageOffset;
}
if (!setAsDefault)
globalConfig->pageOffset = pageOffset;
if (!setAsDefault) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the setAsDefault was the way of distinguishing embedded/windowed
Having now an additional "embedded" makes a mess of the concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. as explained above there are cases where pageOffset needs to be changed temporarily, i.e. without updating the profile. That's what setAsDefault is used.

bool embedded = pdfdocument->embeddedMode;
if (embedded)
pageOffsetEmbedded = pageOffset;
else
globalConfig->pageOffset = pageOffset;
}

if (!refresh)
return;
Expand Down Expand Up @@ -2132,16 +2146,23 @@ int PDFWidget::getPageOffset() const

void PDFWidget::setGridSize(int gx, int gy, bool setAsDefault)
{
bool embedded = pdfdocument->embeddedMode;
if (gridx == gx && gridy == gy)
return;
gridx = gx;
gridy = gy;
if (gridx == 1)
if (gridx == 1) {
setPageOffset(0, true, true);
else if (gridx == 2 && gridy == 1)
}
else if (gridx == 2 && gridy == 1) {
setPageOffset(1, false, true);
else
}
else if (!embedded) {
setPageOffset(globalConfig->pageOffset, true, true);
}
else {
setPageOffset(pageOffsetEmbedded, true, true);
}

if (setAsDefault)
return;
Expand Down Expand Up @@ -2199,7 +2220,15 @@ int PDFWidget::pageStep()
*/
int PDFWidget::gridCols(bool fromConfig) const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like above
the concept from/to config and embedded/windowed needs to cleaned up

{
int result= fromConfig ? globalConfig->gridx : gridx;
int result;
if (!fromConfig)
result = gridx;
else {
if (pdfdocument->embeddedMode)
result = gridxEmbedded;
else
result = globalConfig->gridx;
}
return result;
}
/*!
Expand All @@ -2209,7 +2238,15 @@ int PDFWidget::gridCols(bool fromConfig) const
*/
int PDFWidget::gridRows(bool fromConfig) const
{
int result= fromConfig ? globalConfig->gridy : gridy;
int result;
if (!fromConfig)
result = gridy;
else {
if (pdfdocument->embeddedMode)
result = gridyEmbedded;
else
result = globalConfig->gridy;
}
return result;
}

Expand Down Expand Up @@ -2453,7 +2490,7 @@ void PDFWidget::fitTextWidth(bool checked)
if (!textRect.isValid()) return;
qreal targetWidth = maxPageSizeFDpiAdjusted().width() * (gridx - 1) + textRect.width() * dpi / 72.0;
//qreal targetWidth = textRect.width() * dpi / 72.0;
// total with of all pages in the grid - textMargin of a single page
// total width of all pages in the grid - textMargin of a single page
// for a 1x grid, targetWith is the same as textRect.width()
scaleFactor = portWidth / ((targetWidth ) + 2 * margin);
if (scaleFactor < kMinScaleFactor)
Expand All @@ -2478,8 +2515,11 @@ void PDFWidget::fitWindow(bool checked)
PDFScrollArea *scrollArea = getScrollArea();
if (scrollArea && !pages.isEmpty()) {
qreal portWidth = scrollArea->viewport()->width() - GridBorder * (gridx - 1);
int gy=globalConfig->gridy;
if(pdfdocument->embeddedMode) gy=1;
int gy;
if(pdfdocument->embeddedMode)
gy=gridyEmbedded;
else
gy=globalConfig->gridy;
qreal portHeight = scrollArea->viewport()->height() - GridBorder * (gy - 1); // use globalConfig->gridy as gridy is automatically increased in continous mode to force rendering of surrounding pages
QSizeF pageSize = maxPageSizeFDpiAdjusted();
qreal sfh = portWidth / pageSize.width() / gridx;
Expand Down Expand Up @@ -2919,7 +2959,7 @@ PDFDocument::~PDFDocument()
/*!
* \brief setup ToolBar
*/
void PDFDocument::setupToolBar(){
Copy link
Member

@sunderme sunderme Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused, please remove change (here it shows the original side of code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing argument embedded of method done

void PDFDocument::setupToolBar(bool embedded){
toolBar = new QToolBar(this);
toolBar->setWindowTitle(tr("Toolbar"));
toolBar->setObjectName(QString("toolBar"));
Expand Down Expand Up @@ -2949,7 +2989,7 @@ void PDFDocument::setupToolBar(){
toolBar->addAction(actionFit_to_Text_Width);
toolBar->addAction(actionFit_to_Window);
toolBar->addSeparator();
toolBar->addAction(actionAutoHideToolbars);
toolBar->addAction(actionAutoHideToolbars);
toolBar->addAction(actionEnlargeViewer);
toolBar->addAction(actionShrinkViewer);
toolBar->addAction(actionToggleEmbedded);
Expand Down Expand Up @@ -3072,6 +3112,9 @@ void PDFDocument::setupMenus(bool embedded)
actionGroupGrid->addAction(actionCustom);
menuGrid->addSeparator();
actionSinglePageStep=configManager->newManagedAction(menuroot,menuGrid, "singlePageStep", tr("Single Page Step"), pdfWidget, SLOT(setSinglePageStep(bool)), QList<QKeySequence>());
// if (embedded) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused code, please remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

menuGrid->addAction(actionContinuous);
// }
menuWindow->addAction(menuShow->menuAction());
#if (QT_VERSION > 0x050a00) && (defined(Q_OS_MAC))
actionCloseElement=configManager->newManagedAction(menuroot,menuWindow, "closeElement", tr("&Close something"), this, SLOT(closeElement()), QList<QKeySequence>()); // osx work around
Expand Down Expand Up @@ -3144,10 +3187,9 @@ void PDFDocument::init(bool embedded)
pdfWidget = new PDFWidget(embedded); // needs to be initialized before setup menu
pdfWidget->setPDFDocument(this);

//if (!embedded)
setupMenus(embedded);

setupToolBar();
setupToolBar(embedded);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done



setAttribute(Qt::WA_DeleteOnClose, true);
Expand Down Expand Up @@ -3369,10 +3411,13 @@ void PDFDocument::init(bool embedded)
conf->registerOption("Preview/Continuous", &globalConfig->continuous, true);
conf->linkOptionToObject(&globalConfig->continuous, actionContinuous, LO_NONE);
} else {
pdfWidget->setGridSize(1, 1, true);
pdfWidget->setPageOffset(0, true);
pdfWidget->setSinglePageStep(true);
pdfWidget->setGridxEmbedded(1);
pdfWidget->setGridyEmbedded(1);
pdfWidget->setGridSize(pdfWidget->getGridxEmbedded(), pdfWidget->getGridyEmbedded(), true);
pdfWidget->setPageOffsetEmbedded(0);
pdfWidget->setSinglePageStep(false);
scrollArea->setContinuous(true);
connect(actionContinuous, SIGNAL(toggled(bool)), scrollArea, SLOT(setContinuous(bool)));
}

//connect(actionZoom_In, SIGNAL(triggered()), pdfWidget, SLOT(zoomIn()));
Expand Down Expand Up @@ -3696,11 +3741,11 @@ void PDFDocument::loadCurrentFile(bool fillCache)
break; // message is handled via messageFrame
}
pdfWidget->hide();
pdfWidget->setDocument(document);
pdfWidget->setDocument(document, embeddedMode);
if (error == PDFRenderManager::FileIncomplete)
reloadWhenIdle();
} else {
pdfWidget->setDocument(document);
pdfWidget->setDocument(document, embeddedMode);
pdfWidget->show();

annotations = new PDFAnnotations(this);
Expand Down Expand Up @@ -3805,11 +3850,20 @@ void PDFDocument::setGrid()
d.addVariable(&y , "Y-Grid:");
if (d.exec()) {
pdfWidget->setGridSize(x, y);
globalConfig->gridx = x;
globalConfig->gridy = y;
if (embeddedMode) {
pdfWidget->setGridxEmbedded(x);
pdfWidget->setGridyEmbedded(y);
} else{
globalConfig->gridx = x;
globalConfig->gridy = y;
}
}
// set grid menu entry checked
QString gs=QString("%1x%2").arg(globalConfig->gridx).arg(globalConfig->gridy);
QString gs;
if (embeddedMode)
gs=QString("%1x%2").arg(pdfWidget->getGridxEmbedded()).arg(pdfWidget->getGridyEmbedded());
else
gs=QString("%1x%2").arg(globalConfig->gridx).arg(globalConfig->gridy);
bool found=false;
for(QAction *a:actionGroupGrid->actions()){
if(a->property("grid").toString()==gs){
Expand All @@ -3824,9 +3878,16 @@ void PDFDocument::setGrid()
}
} else {
int p = gs.indexOf("x");
globalConfig->gridx = gs.left(p).toInt();
globalConfig->gridy = gs.mid(p + 1).toInt();
pdfWidget->setGridSize(globalConfig->gridx, globalConfig->gridy);
int x = gs.left(p).toInt();
int y = gs.mid(p + 1).toInt();
pdfWidget->setGridSize(x, y);
if (embeddedMode) {
pdfWidget->setGridxEmbedded(x);
pdfWidget->setGridyEmbedded(y);
} else{
globalConfig->gridx = x;
globalConfig->gridy = y;
}
}
pdfWidget->windowResized();
}
Expand Down
21 changes: 15 additions & 6 deletions src/pdfviewer/PDFDocument.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class PDFWidget : public QLabel
explicit PDFWidget(bool embedded = false);
virtual ~PDFWidget();

void setDocument(const QSharedPointer<Poppler::Document> &doc);
void setDocument(const QSharedPointer<Poppler::Document> &doc, bool embedded);
void setPDFDocument(PDFDocument *docu);

void saveState(); // used when toggling full screen mode
Expand Down Expand Up @@ -232,6 +232,14 @@ class PDFWidget : public QLabel
Q_INVOKABLE void zoom(qreal scale);

virtual void wheelEvent(QWheelEvent *event);
int getGridxEmbedded() {return gridxEmbedded;};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this concepts does not comply with the general idea of object oriented programming.
You should not have special functions to set normal parameters when the object is just in a different state (i.e. embedded)
This way the state needs to be tracked in the caller & the receiver and you can make mistake on both ends.

So the issue is that you want to overwrite the global config for the grid.
The simplest way, is to keep it as it is:

  • grid 1/1 does not overwrite global config
  • other grid settings set the global config and thus stay when you switch from embedded to windowed.
  • This seems like a useful behavior

Copy link
Contributor Author

@octaeder octaeder Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of setting grid to 1x1 without changes to the original config turns out to be problematic. When continuous mode is switched off, the grid size is restored from the config, resulting in unexpected grid changes for the user. This is independent of whether the imbedded and windowed viewer use the same or different configurations.
Both embedded and windowed viewer initialize the config to grid 1x1 when startet first time ever. Regarding your concerns about changes to the grid, I would say that this is not such a problem. An unexperienced user might do grid changes and others with the main menu in the windowed viewer. This would have no effect on the embedded viewer (assuming two configs). Only when he discovers the context menu can he make changes in this case. I think this is acceptable. Restoring the settings of the embedded viewer will certainly please many users.
This is now the current PR.

int getGridyEmbedded() {return gridyEmbedded;};
int getPageOffsetEmbedded() {return pageOffsetEmbedded;};
bool getSinglePageStepEmbedded() {return singlePageStepEmbedded;};
void setGridxEmbedded(int x) {gridxEmbedded=x;};
void setGridyEmbedded(int y) {gridyEmbedded=y;};
void setPageOffsetEmbedded(int o) {pageOffsetEmbedded=o;};
void setSinglePageStepEmbedded(bool s) {singlePageStepEmbedded=s;};

protected slots: //not private, so scripts have access
void goFirst();
Expand Down Expand Up @@ -324,7 +332,6 @@ public slots:
QSharedPointer<Poppler::Document> document;
QMutex textwidthCalculationMutex;

//QList<int> pages;
QSharedPointer<Poppler::Link> clickedLink;
QSharedPointer<Poppler::Annotation> clickedAnnotation;

Expand Down Expand Up @@ -363,9 +370,10 @@ public slots:
#endif
int currentTool; // the current tool selected in the toolbar
int usingTool; // the tool actually being used in an ongoing mouse drag
bool singlePageStep;
bool singlePageStep, singlePageStepEmbedded;

int gridx, gridy, pageOffset;
int gridxEmbedded, gridyEmbedded, pageOffsetEmbedded;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the need for extra variables to keep a state (grid setting)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a missunderstanding. The Embedded ones are used similar to the ones from the config. Why this? Well, option continuous changes temporarily gridy (and similar single page step changes offset temporarily). So, turning of this option makes it necessary to retrieve the original values (I do so for gridx also, even so it is not needed). Until now this has been gridy=1. But now we need the user supplied value. This could be the config. Maybe I didn't get right, what you said here #3554 (comment). Anyway, I still think that the user may want to have two non-shared settings, because one preferes other settings in the smaler area of the embedded viewer. If a future decision is to save the embedded settings, too, then this could be done easier.


bool forceUpdate;

Expand Down Expand Up @@ -561,8 +569,8 @@ private slots:

private:
void init(bool embedded = false);
void setupMenus(bool embedded);
void setupToolBar();
void setupMenus(bool embedded);
void setupToolBar(bool embedded);
void setCurrentFile(const QString &fileName);
void loadSyncData();

Expand Down Expand Up @@ -590,7 +598,6 @@ private slots:
QMenu *menuFile;
QMenu *menuEdit;
QMenu *menuView;
QMenu *menuGrid;
QMenu *menuWindow;
QList<QMenu *>menus;

Expand Down Expand Up @@ -675,8 +682,10 @@ private slots:

QStatusBar *statusbar;
QToolBar *toolBar;
QToolBar *tbPdfView;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused, please remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, and change to definition of setupToolBar must also be removed. This is done.

QTimer *toolBarTimer;
public:
QMenu *menuGrid;
QMenu *menuShow;
private:
QMenu *menuEdit_2;
Expand Down
Loading