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.

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

Gui: fix possibly garbled toolbars and menus #8783

Merged
merged 1 commit into from Mar 9, 2023

Conversation

wwmayer
Copy link
Contributor

@wwmayer wwmayer commented Mar 8, 2023

See also Debian bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=986821 For now enable this line only for Windows builds

Thank you for creating a pull request to contribute to FreeCAD! Place an "X" in between the brackets below to "check off" to confirm that you have satisfied the requirement, or ask for help in the FreeCAD forum if there is something you don't understand.

  • Your Pull Request meets the requirements outlined in section 5 of CONTRIBUTING.md for a Valid PR

Please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.


See also Debian bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=986821
For now enable this line only for Windows builds
@github-actions github-actions bot added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Mar 8, 2023
@freecadci
Copy link

pipeline status for feature branch PR_8783. Pipeline 800050980 was triggered at d32bc0d. All CI branches and pipelines.

@wwmayer wwmayer merged commit f70bf28 into FreeCAD:master Mar 9, 2023
6 checks passed
@wwmayer wwmayer deleted the fix_scale_factor_rounding branch March 9, 2023 11:44
@xtemp09
Copy link
Contributor

xtemp09 commented Mar 9, 2023

Can I ask here a question closely related to this kind of style?

Would it be better to define global constants to simplify the code?

Like this:

#ifdef Q_OS_WIN
    bool isOSWindows = true;
    bool isOSLinux = false;
    bool isOSAndroid = false;
#endif

//<some code>
namespace Base::Qt {
    int major = 5;
    int minor = 15;
    int patch = 7;
}

The code you've just sent becomes:

if constexpr ( isOSWindows && (Base::Qt::major == 5) && (Base::Qt::minor > 14) )
        QGuiApplication::setHighDpiScaleFactorRoundingPolicy(Qt::HighDpiScaleFactorRoundingPolicy::PassThrough);

Therefore, this fragment in /src/Mod/Fem/Gui/DlgSettingsFemElmerImp.cpp

fragment

#if defined(FC_OS_WIN32)
    // name ends with "_mpi.exe"
    if (!FileName.endsWith(QLatin1String("_mpi.exe"))) {
        QMessageBox::warning(this, tr("FEM Elmer: Not suitable for multithreading"),
            tr("Wrong Elmer setting: You use more than one CPU core.\n"
                "Therefore an executable with the suffix '_mpi.exe' is required."));
        ui->sb_elmer_num_cores->setValue(1);
        ui->sb_elmer_num_cores->setMaximum(1);
        return;
    }
#elif defined(FC_OS_LINUX) || defined(FC_OS_CYGWIN) || defined(FC_OS_MACOSX) || defined(FC_OS_BSD)
    // name ends with "_mpi"
    if (!FileName.endsWith(QLatin1String("_mpi"))) {
        QMessageBox::warning(this, tr("FEM Elmer: Not suitable for multithreading"),
            tr("Wrong Elmer setting: You use more than one CPU core.\n"
                "Therefore an executable with the suffix '_mpi' is required."));
        ui->sb_elmer_num_cores->setValue(1);
        ui->sb_elmer_num_cores->setMaximum(1);
        return;
    }
#endif

becomes:

const QString ending = [] (bool os) {
	if (os) return QLatin1String("_mpi.exe");
	else    return QLatin1String("_mpi"); } (isOSWindows);

if ( !FileName.endsWith(ending) ) {
	QMessageBox::warning( this, tr("FEM Elmer: Not suitable for multithreading"),
	tr("Wrong Elmer setting: You use more than one CPU core.\n"
	"Therefore an executable with the suffix '%1' is required.").arg( ending ) );
	ui->sb_elmer_num_cores->setValue(1);
	ui->sb_elmer_num_cores->setMaximum(1);
    }

Usage of constexpr also would be good.

@xtemp09
Copy link
Contributor

xtemp09 commented Mar 9, 2023

I wrote the previous post in the comment field out of frustration, so it should be viewed as an idea. By no means I'm trying to reprimand, I'm just thinking to ease understanding of the code. The code of FreeCAD is complex. Usage of constexpr and if constexpr keywords will simplify it. I'm thinking of the following:

if constexpr ( Base::sysInfo::isOSWindows() ){
	SetProcessDPIAware();
}
if constexpr ( isOSWindows ){
	if constexpr ( Base::QInfo::Version.major < 6){
            QCoreApplication::setAttribute(Qt::AA_EnableHighDpiScaling);
	    QCoreApplication::setAttribute(Qt::AA_UseHighDpiPixmaps);
	}
	
	if constexpr (  Base::QInfo::Version.major == 5 &&
			Base::QInfo::Version.minor > 14 ){
        QGuiApplication::setHighDpiScaleFactorRoundingPolicy(Qt::HighDpiScaleFactorRoundingPolicy::PassThrough);
        }

	if constexpr (  Base::QInfo::Version.major == 5 &&
			Base::QInfo::Version.minor == 10 ){
        QCoreApplication::setAttribute(Qt::AA_CompressTabletEvents);
        }
}

@wwmayer
Copy link
Contributor Author

wwmayer commented Mar 10, 2023

Will such code still compile with Qt6 then? Some of the Qt::AA attributes are not available in Qt6 any more and thus I don't think that if constexpr ( Base::QInfo::Version.major < 6){ will work because constexpr does not guarantee that the code will be evaluated at compile time. It can but doesn't have to.

@xtemp09
Copy link
Contributor

xtemp09 commented Mar 11, 2023

Well, it requires some digging. If FreeCAD is being ported to Qt 6, might I ask how QWinTaskbarButton is going to be replaced? It was removed in Qt 6.

I can speak for all Windows users that observing progress of heavy tasks in task bar is incredibly good feature.

@wwmayer
Copy link
Contributor Author

wwmayer commented Mar 11, 2023

Apparently there are plans to provide this feature for all platforms. So, it will be part of the official Qt API. But I don't know when this will happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants