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

octaeder · 2025-01-10T16:16:13Z

Happy new year!

This PR resolves #222 (enhancement request). The changes add a Grid menu to the context menu of both the windowed and the embedded pdf-viewer. The Grid menu offers known options from the main menu View of the windowed pdf-viewer. The menu then looks like this:

This may often allow working without the windowed pdf-viewer. For example, you could choose grid 2x1 and option Fit to Window. If you want to take a closer look at the pdf, you can expand the viewer to the left (and later on shrink it to the right) with the button Enlarge Viewer to the right of the tool bar. On large screens it may suffice to enlarge the texstudio window.

Following assertions hold:

A new embedded viewer (or a windowed viewer getting embedded) always sets the options as shown in the image (including Fit to Width). These are the ones used before this PR (c.f. #3554#issuecomment-2001989472).
Only the windowed viewer saves and restores previously set options.
These options of embedded and windowed viewer do not interfere.

Previous PR is #3554.

Note: Changed PR description after discussion.

sunderme · 2025-01-10T17:38:35Z

I have two points

the button should be a symbol, not "grid". It works in English but less so in other languages.
I am really not convinced that it should be placed prominently.
How about adding it to context menu (next to show or in show) ?

octaeder · 2025-01-10T18:05:39Z

context menu would be ok. So we would have a sequence of seperator, Grid, Show, right?

sunderme · 2025-01-10T18:29:31Z

context menu would be ok. So we would have a sequence of seperator, Grid, Show, right?

correct

octaeder · 2025-01-10T19:39:53Z

Grid to context menu only for embedded mode?

octaeder · 2025-01-10T19:51:29Z

Grid to context menu only for embedded mode?

I would prefere always having Grid in both modes. On large monitors, this can reduce mouse pointer movements.

octaeder · 2025-01-10T19:59:34Z

and for continuous I suggest the same. So you don't need to think about which action where to find in different modes.

sunderme · 2025-01-10T20:12:53Z

I don't see a benefit of having non-continuous or single step in the embedded viewer. But if it must be, maybe part of the grid sub menu

----
Grid
  1x1
  ...
  nxm
  ----
  continuous
  etc.
Show
  ...

octaeder · 2025-01-10T20:20:16Z

well that structure is already as you describe it. Non-continous allows faster and precise scrolling with the wheel. The question is should continous also be there in the windowed case. As said, I would prefere it.

octaeder · 2025-01-10T20:31:39Z

so windowed and embedded viewer:

sunderme · 2025-01-10T20:34:16Z

that is fine

octaeder · 2025-01-10T20:39:50Z

and that's why I suggested to move continuous from View to View/Grid in last PR. Would you change your mind now?

sunderme · 2025-01-10T20:49:04Z

and that's why I suggested to move continuous from View to View/Grid in last PR. Would you change your mind now?

The previous suggestion was about the pdf main menu , yet here we are talking about context menu , right ?
I still see no reason to change the pdf main menu.

octaeder · 2025-01-10T22:41:28Z

I think nothing more to do from my side

sunderme

There needs to be a cleaner concept about embedded/windowed handling of the grid setting.

And don't leave obsolete/dead code in pull-requests

sunderme · 2025-01-11T09:56:07Z

src/pdfviewer/PDFDocument.cpp

@@ -2919,7 +2959,7 @@ PDFDocument::~PDFDocument()
 /*!
 * \brief setup ToolBar
 */
-void PDFDocument::setupToolBar(){


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

removing argument embedded of method done

sunderme · 2025-01-11T09:56:22Z

src/pdfviewer/PDFDocument.cpp

@@ -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) {


unused code, please remove

sunderme · 2025-01-11T09:56:35Z

src/pdfviewer/PDFDocument.cpp

    setupMenus(embedded);

-    setupToolBar();
+    setupToolBar(embedded);


this is done

sunderme · 2025-01-11T10:06:59Z

src/pdfviewer/PDFDocument.h

@@ -232,6 +232,14 @@ class PDFWidget : public QLabel
 	Q_INVOKABLE void zoom(qreal scale);

 	virtual void wheelEvent(QWheelEvent *event);
+	int getGridxEmbedded() {return gridxEmbedded;};


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

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.

sunderme · 2025-01-11T10:08:32Z

src/pdfviewer/PDFDocument.h

@@ -675,8 +682,10 @@ private slots:

    QStatusBar *statusbar;
    QToolBar *toolBar;
+    QToolBar *tbPdfView;


unused, please remove

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

sunderme · 2025-01-11T10:13:30Z

src/pdfviewer/PDFDocument.h


 	int gridx, gridy, pageOffset;
+	int gridxEmbedded, gridyEmbedded, pageOffsetEmbedded;


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

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.

sunderme · 2025-01-11T10:17:47Z

src/pdfviewer/PDFDocument.cpp

+	if (pdfDoc) {
+		if (pdfDoc->menuGrid || pdfDoc->menuShow) {
+			menu.addSeparator();
+			if (pdfDoc->menuGrid) {
+				menu.addMenu(pdfDoc->menuGrid);
+			}
+			if (pdfDoc->menuShow) {
+				menu.addMenu(pdfDoc->menuShow);
+			}
+		}


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.

simplified and assinged nullptr in headerfile

sunderme · 2025-01-11T10:19:49Z

src/pdfviewer/PDFDocument.cpp

 	}
-	if (!setAsDefault)
-		globalConfig->pageOffset = pageOffset;
+	if (!setAsDefault) {


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

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.

sunderme · 2025-01-11T10:21:42Z

src/pdfviewer/PDFDocument.cpp

@@ -2199,7 +2220,15 @@ int PDFWidget::pageStep()
 */
 int PDFWidget::gridCols(bool fromConfig) const


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

sunderme · 2025-01-13T08:37:32Z

Thanks

add toolbutton grid actions to embedded pdf-viewer

7f83e03

add Grid menu to context menu, no toolbar button

043ec94

sunderme requested changes Jan 11, 2025

View reviewed changes

octaeder marked this pull request as draft January 11, 2025 17:21

octaeder added 2 commits January 11, 2025 18:42

removing unnecessary changes

1a65c06

use partly seperate config for embedded viewer

59a4ca1

octaeder force-pushed the #222-gridImbeddedPdf branch from e25f20e to 59a4ca1 Compare January 12, 2025 04:15

further simplify if conditions

b971f45

octaeder force-pushed the #222-gridImbeddedPdf branch from 5ee7cba to b971f45 Compare January 12, 2025 14:01

octaeder marked this pull request as ready for review January 12, 2025 18:18

factor commen string

6ad8a1a

sunderme merged commit 2e43e5a into texstudio-org:master Jan 13, 2025
7 checks passed

octaeder deleted the #222-gridImbeddedPdf branch January 13, 2025 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

octaeder commented Jan 10, 2025 •

edited

Loading

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025 •

edited

Loading

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025

octaeder commented Jan 10, 2025

octaeder commented Jan 10, 2025

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025

octaeder commented Jan 10, 2025

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025

sunderme left a comment

sunderme Jan 11, 2025 •

edited

Loading

octaeder Jan 11, 2025

sunderme Jan 11, 2025

octaeder Jan 11, 2025

sunderme Jan 11, 2025

octaeder Jan 11, 2025

sunderme Jan 11, 2025

octaeder Jan 12, 2025 •

edited

Loading

sunderme Jan 11, 2025

octaeder Jan 11, 2025

sunderme Jan 11, 2025

octaeder Jan 11, 2025

sunderme Jan 11, 2025

octaeder Jan 11, 2025

sunderme Jan 11, 2025

octaeder Jan 11, 2025

sunderme Jan 11, 2025

sunderme commented Jan 13, 2025


		int gridx, gridy, pageOffset;
		int gridxEmbedded, gridyEmbedded, pageOffsetEmbedded;

		@@ -2199,7 +2220,15 @@ int PDFWidget::pageStep()
		*/
		int PDFWidget::gridCols(bool fromConfig) const

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

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

Conversation

octaeder commented Jan 10, 2025 • edited Loading

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025 • edited Loading

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025

octaeder commented Jan 10, 2025

octaeder commented Jan 10, 2025

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025

octaeder commented Jan 10, 2025

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025

sunderme commented Jan 10, 2025

octaeder commented Jan 10, 2025

sunderme left a comment

Choose a reason for hiding this comment

sunderme Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octaeder Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunderme commented Jan 13, 2025

octaeder commented Jan 10, 2025 •

edited

Loading

octaeder commented Jan 10, 2025 •

edited

Loading

sunderme Jan 11, 2025 •

edited

Loading

octaeder Jan 12, 2025 •

edited

Loading