Page MenuHomeWildfire Games

Remove obsolete clipboard and GetVideoMode platform-specifics
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
macOS Developers
Trac Tickets
#3145
#5537
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.

Event Timeline

linkmauve created this revision.Dec 17 2019, 12:21 PM

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
602

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

611–612

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

source/ps/VideoMode.h
110

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
602

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
602

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())
{
	// ...
}
605–606

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

611–612

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

source/lib/sysdep/os/win/wsysdep.cpp
213

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–185

Do we have the problem before the patch?

linkmauve added inline comments.Jan 4 2020, 6:39 PM
source/gui/ObjectTypes/CInput.cpp
602

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.

611–612

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–185

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.Thu, Jan 23, 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.Thu, Jan 23, 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.Thu, Jan 23, 3:28 PM
elexis added a subscriber: elexis.EditedThu, Jan 23, 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.Thu, Jan 23, 3:32 PM
elexis updated the Trac tickets for this revision.