Page MenuHomeWildfire Games

Remove obsolete GetVideoMode platform-specifics
ClosedPublic

Authored by linkmauve on Dec 17 2019, 12:21 PM.

Details

Summary

This is now handled in a platform-agnostic way by SDL2’s APIs.

Changes at https://github.com/linkmauve/0ad/tree/rm-sys

Test Plan
  • Check that it builds properly on all platforms.
  • Check that there is no regression, especially around console copy/paste on all platforms.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/777/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/sysdep/os/android/android.cpp
|   1| /*·Copyright·(C)·2012·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2012"

source/lib/res/graphics/cursor.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1293/display/redirect

linkmauve updated this revision to Diff 10629.Dec 18 2019, 12:21 PM
linkmauve retitled this revision from Remove obsolete usage of platform-specific cursors to Remove obsolete usage of platform-specifics.
linkmauve edited the summary of this revision. (Show Details)
linkmauve edited the test plan for this revision. (Show Details)

This now removes almost all platform-specific calls, not just cursors but also clipboards and video modes, replacing them with SDL2 calls.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/780/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/sysdep/gfx.h
|   1| /*·Copyright·(C)·2013·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2013"

source/lib/sysdep/gfx.h
|  30| namespace·gfx·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespacegfx{' is invalid C code. Use --std or --language to configure the language.

source/ps/VideoMode.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.

source/lib/sysdep/os/win/wgfx.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/ps/VideoMode.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/lib/sysdep/os/win/wsysdep.cpp
|   1| /*·Copyright·(C)·2011·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2011"

source/lib/sysdep/os/osx/osx.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/lib/sysdep/gfx.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1296/display/redirect

linkmauve updated this revision to Diff 10630.Dec 18 2019, 12:40 PM

Include back the wcursor removal.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/781/display/redirect

Stan added a subscriber: Stan.Dec 18 2019, 12:47 PM
Stan added inline comments.
source/gui/ObjectTypes/CInput.cpp
601 ↗(On Diff #10630)

Is there a way to limit the number of objects we create?

615 ↗(On Diff #10630)

static_cast<int>(expr) In the new coding conventions :)

source/ps/VideoMode.h
108 ↗(On Diff #10630)

Capital B.
Can you put comments on top ? Optionnally add a final .

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/sysdep/os/osx/osx.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/lib/sysdep/gfx.h
|   1| /*·Copyright·(C)·2013·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2013"

source/lib/sysdep/gfx.h
|  30| namespace·gfx·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespacegfx{' is invalid C code. Use --std or --language to configure the language.

source/lib/sysdep/os/win/wgfx.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/lib/res/graphics/cursor.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/ps/VideoMode.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.

source/lib/sysdep/gfx.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/ps/VideoMode.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/lib/sysdep/os/win/wsysdep.cpp
|   1| /*·Copyright·(C)·2011·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2011"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1297/display/redirect

linkmauve added inline comments.Dec 18 2019, 1:09 PM
source/gui/ObjectTypes/CInput.cpp
601 ↗(On Diff #10630)

This is rare enough, not worth optimising for one less allocation for now. But we should eventually migrate from std::wstring to always using UTF-8-encoded strings.

linkmauve updated this revision to Diff 10631.Dec 18 2019, 1:10 PM
linkmauve retitled this revision from Remove obsolete usage of platform-specifics to Remove obsolete platform-specifics.

Fix Stan`’s comments:

  • Describe better the content of frequency and bpp.
  • Change C-style casts into static_casts.
linkmauve marked 2 inline comments as done.Dec 18 2019, 1:11 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/782/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/VideoMode.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.

source/lib/sysdep/gfx.h
|   1| /*·Copyright·(C)·2013·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2013"

source/lib/sysdep/gfx.h
|  30| namespace·gfx·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespacegfx{' is invalid C code. Use --std or --language to configure the language.

source/lib/sysdep/os/win/wgfx.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/lib/sysdep/gfx.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/lib/res/graphics/cursor.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/lib/sysdep/os/osx/osx.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/ps/VideoMode.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/lib/sysdep/os/win/wsysdep.cpp
|   1| /*·Copyright·(C)·2011·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2011"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1298/display/redirect

Stan added a comment.Dec 18 2019, 1:56 PM

Tested copy paste and cursor on windows everything works fine.

Stan added a comment.Dec 18 2019, 5:04 PM

Tested by vv22 on X11 with debian sid. Copy paste and cursor work ;)

I'd like to split the patch for an easier review and commit search.

linkmauve updated this revision to Diff 10784.Dec 25 2019, 3:45 PM
  • Mention in clipboard commit that it fixes #5537.
  • Update the copyright year of touched files.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/887/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/sysdep/gfx.h
|  30| namespace·gfx·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespacegfx{' is invalid C code. Use --std or --language to configure the language.

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1403/display/redirect

vladislavbelov added inline comments.Jan 3 2020, 9:08 PM
source/gui/ObjectTypes/CInput.cpp
601 ↗(On Diff #10784)

I don't think that we need the text variable after that line. I suggest to write it like:

std::wstring text;
if (char* clipboard_text = SDL_GetClipboardText())
{
	text = wstring_from_utf8(clipboard_text);
	SDL_free(text);
}
if (!text.empty())
{
	// ...
}
607 ↗(On Diff #10784)

We can use std::wstring directly here and below.

615 ↗(On Diff #10784)

Are you sure that wcslen(text.c_str()) != text.size()?

source/lib/sysdep/os/win/wsysdep.cpp
213 ↗(On Diff #10784)

It seems that it breaks the copy button in the error dialog (when you got a crash/assertion) on Windows.

source/ps/VideoMode.cpp
178 ↗(On Diff #10784)

Do we have the problem before the patch?

linkmauve added inline comments.Jan 4 2020, 6:39 PM
source/gui/ObjectTypes/CInput.cpp
601 ↗(On Diff #10784)

I originally did that to minimise the amount of changes per commit, since this is already quite a lot to review, but I’ll change it in the next revision.

615 ↗(On Diff #10784)

I’m still very uneasy with the difference between LP64 and LLP64 in this area, but it may give the correct result.

source/ps/VideoMode.cpp
178 ↗(On Diff #10784)

Yes we do, at least on Linux, we only ever consider the default screen (whatever that means).

Stan edited the summary of this revision. (Show Details)Jan 6 2020, 10:44 PM

I'm going to commit removing of the system cursor soon.

linkmauve marked 4 inline comments as done.Jan 23 2020, 11:49 AM

The latest changes are rebased and pushed at https://github.com/linkmauve/0ad/tree/rm-sys

linkmauve updated this revision to Diff 11155.Jan 23 2020, 12:44 PM

Fix Vladislav’s comments, still mirrored at GitHub to avoid squashing the commits.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1117/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.

source/lib/sysdep/gfx.h
|  30| namespace·gfx·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespacegfx{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1635/display/redirect

elexis updated the Trac tickets for this revision.Jan 23 2020, 3:28 PM
elexis added a subscriber: elexis.EditedJan 23 2020, 3:30 PM

This patch fixes the "copy twice" bug described in #3145!
And the crash over there can't be reproduced by me on arch, nor by go2die on mint 19.3 Tricia (based on ubuntu bionic 18.04.3. LTS kernel 5.3.x) (see lobby today)

Edit: bb tested as well on fedora 29 (see irc today) and also cant reproduce the crash

elexis retitled this revision from Remove obsolete platform-specifics to Remove obsolete clipboard and GetVideoMode platform-specifics.Jan 23 2020, 3:32 PM
elexis updated the Trac tickets for this revision.
Angen added a subscriber: Angen.Feb 21 2020, 11:01 PM

@elexis
He meant the button on crash window, not that it creates crash.

Angen requested changes to this revision.Feb 21 2020, 11:13 PM

Failing to build on windows.

16>..\..\..\source\lib\sysdep\os\win\wsysdep.cpp(217): error C3861: 'SDL_SetClipboardText': identifier not found [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vc2015\lowlevel.vcxproj]
This revision now requires changes to proceed.Feb 21 2020, 11:13 PM
Stan added inline comments.Feb 22 2020, 10:12 AM
source/lib/sysdep/os/win/wsysdep.cpp
38 ↗(On Diff #11155)

Likely missing an SDL include.

linkmauve updated this revision to Diff 11418.Feb 24 2020, 8:36 PM

Add missing SDL_clipboard.h include for Windows.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.

source/lib/sysdep/gfx.h
|  30| namespace·gfx·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespacegfx{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1809/display/redirect

Angen resigned from this revision.Feb 24 2020, 9:00 PM

windows build was fixed

source/lib/sysdep/os/win/wsysdep.cpp
213 ↗(On Diff #10784)

copy button copies properly

Stan updated this revision to Diff 11793.May 5 2020, 2:44 PM

Rebase after rP23624 & rP23625

Vulcan added a comment.May 5 2020, 2:44 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2074/display/redirect

Vulcan added a comment.May 5 2020, 2:47 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1548/display/redirect

Vulcan added a comment.May 5 2020, 2:48 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/659/display/redirect

Stan updated this revision to Diff 11794.May 5 2020, 2:49 PM

Actually rebase

Vulcan added a comment.May 5 2020, 2:58 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/sysdep/gfx.h
|  30| namespace·gfx·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespacegfx{' is invalid C code. Use --std or --language to configure the language.

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2075/display/redirect

Vulcan added a comment.May 5 2020, 3:25 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/660/display/redirect

Stan retitled this revision from Remove obsolete clipboard and GetVideoMode platform-specifics to Remove obsolete GetVideoMode platform-specifics.May 19 2020, 9:51 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 8 2020, 7:49 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.