Merged
Conversation
Fixes: cztomczak#546 Had to include harfbuzz manually as newer Pango change this. See: eiskaltdcpp/eiskaltdcpp#413 https://gitlab.gnome.org/GNOME/pango/-/issues/387 Also had to add `-Wno-deprecated-declarations` to get this to compile because of the following errors that didn't seem to be coming from this code directly: warning: ‘GTimeVal’ is deprecated: Use 'GDateTime' instead warning: ‘GTypeDebugFlags’ is deprecated
…k#484). These callbacks were never called previously. Rename --no-run-examples flag to --unittests in build scripts.
… code at frame.pyx
|
when this can merge and publish one package? users are waiting |
|
Thanks for the port to 123. I think that this repository is no longer supported. Or may be wright small build instruction. I try build your fork with I download and unpack to And got this: Outputif you have any suggestion I will be grateful |
Contributor
Author
Are you able to use newer windows 10 sdk? This thread also suggests newer win10 sdk https://www.magpcss.org/ceforum/viewtopic.php?f=6&t=19436 |
Thank you! This version helped me. With this lib compiled and works. |
cztomczak
requested changes
Feb 7, 2025
Owner
cztomczak
left a comment
cztomczak
left a comment
There was a problem hiding this comment.
Thank you for your contribution @linesight. You can find my code comments attached to this review.
| pyBrowser.GetMainFrame().SendProcessMessage(cef_types.PID_RENDERER, | ||
| 0, "DoJavascriptBindings", [{ | ||
| mainFrame = pyBrowser.GetMainFrame() | ||
| if mainFrame.IsValid(): |
Owner
There was a problem hiding this comment.
Should we print a log message when frame is invalid?
Contributor
Author
There was a problem hiding this comment.
I don't have a strong preference log or not log when frame is invalid
Owner
|
Also @linesight , why does GitHub show me "This branch has conflicts that must be resolved" message? |
Contributor
Author
|
I think I have addressed most (if not all ) techinical feedback from @cztomczak , really appreciate you also double check at upstream cef |
cztomczak
reviewed
Feb 17, 2025
cztomczak
reviewed
Feb 17, 2025
cztomczak
reviewed
Feb 17, 2025
cztomczak
reviewed
Feb 17, 2025
cztomczak
reviewed
Feb 17, 2025
cztomczak
reviewed
Feb 17, 2025
|
|
||
| cdef object GetUniqueFrameId(int browserId, object frameId): | ||
| return str(browserId) +"#"+ str(frameId) | ||
| cdef str GetUniqueFrameId(int browserId, py_string frameId): |
Owner
There was a problem hiding this comment.
I guess previously it used object because of py2 <> py3 string differences. Hmm now we use Py2 in cython and Py3 for apps and this works fine?
Contributor
Author
There was a problem hiding this comment.
frameId is now of py_string , to align with how current upstream cef123 is treating frame id as a string, see
chromiumembedded/cef@2f1e782
I don't have histocial knowledge why object type was used, but I guess it was due to old frame id being int64_t type, didn't have nice or convenient mapping in cython
as frameId is now of string type, it is natural to change to function GetUniqueFrameId return type to string as well. More specifc type is, in general, more preferred than general type object.
For "Py2 in cython" comment, I think you are referring to
"language_level": 2,
still being used in cython_setup.py
I noticed this line at the beginning of development of this pull request, I tried to get rid of this line at first, then I met warning
cythoning cefpython_py311.pyx to cefpython_py311.cpp
D:\workspace\cefpython\venv311\Lib\site-packages\Cython\Compiler\Main.py:369: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: D:\workspace\cefpython\build\build_cefpython\cefpython_py311.pyx
tree = Parsing.p_module(s, pxd, full_module_name)
I didn't mean to introduce extra warning, then I put this language_level line back. How cython deals with this language_level internally isn't my primrary concern. Suppose you use python3 to execute this build instruction and example code, things work fine
Owner
There was a problem hiding this comment.
language_level can be set to 2 or 3 or even 3str. Changing it would require extensive testing of all APIs to make sure nothing got broken.
language_level (2/3/3str), default=None
Globally set the Python language level to be used for module compilation. Default is None, indicating compatibility with Python 3 in Cython 3.x and with Python 2 in Cython 0.x. To enable Python 3 source code semantics, set this to 3 (or 3str) at the start of a module or pass the “-3” or “–3str” command line options to the compiler. For Python 2 semantics, use 2 and “-2” accordingly. Before Cython 3.1, the 3str option enabled Python 3 semantics but did not change the str type and unprefixed string literals to unicode when the compiled code runs in Python 2.x. In Cython 3.1, 3str is an alias for 3. Language level 2 ignores x: int type annotations due to the int/long ambiguity. Note that cimported files inherit this setting from the module being compiled, unless they explicitly set their own language level. Included source files always inherit this setting.
Ref:
Owner
There was a problem hiding this comment.
In requirements.txt Cython version is Cython == 0.29.36. That version supports only language level 2 (Python 2).
I just checked and see latest version is Cython 3.0.12 which supports language level 3 and 3str:
https://github.com/cython/cython/releases
Owner
There was a problem hiding this comment.
In Cython i recall much confusion with str/unicode types. When you define function returning str type it really is Python 2 byte string from what I recall. To return unicode string I think that;s the reason for object type - when project had to support both Python 2 and Python 3. I'm not sure now, but it was all confusing.
Owner
There was a problem hiding this comment.
More on Cython strings: https://cython.readthedocs.io/en/stable/src/tutorial/strings.html
Cython supports four Python string types: bytes, str, unicode and basestring...
A nice read 😊
Owner
There was a problem hiding this comment.
Changing cython language level might change how strings are handled, or other things I don't know. I dont recommend changing it without re-testing of all APIs. Our unit tests are limited.
Contributor
Author
There was a problem hiding this comment.
Thanks for the detailed info. For the line of code in question, do you prefer me changing back to
cdef object GetUniqueFrameId(int browserId, str frameId):
or we can live with
cdef str GetUniqueFrameId(int browserId, py_string frameId):
?
I don't have strong preference of either way, I just slightly prefer str over object
Owner
There was a problem hiding this comment.
It's a cdef, so internal function. It doesn't much matter here. What matters is what you expose to Python API - a byte string or unicode string - we should be consistent with the rest of the API.
cztomczak
reviewed
Feb 17, 2025
| @@ -80,7 +80,7 @@ | |||
| ALLOW_PARTIAL = False | |||
|
|
|||
| # Python versions | |||
| SUPPORTED_PYTHON_VERSIONS = [(2, 7), (3, 4), (3, 5), (3, 6), (3, 7), (3, 8), (3, 9)] | |||
| SUPPORTED_PYTHON_VERSIONS = [(2, 7), (3, 4), (3, 5), (3, 6), (3, 7), (3, 8), (3, 9), (3, 10), (3, 11)] | |||
Owner
There was a problem hiding this comment.
We should drop support Python 2.7. Also old Python 3 version such as 3.4, 3.5, 3.6.... we don't need that. We should only support latest popular Py 3 versions.
Owner
There was a problem hiding this comment.
Just for information. This PR is about CEF upgrade. Support in regards to Python versions is a separate issue.
Contributor
Author
There was a problem hiding this comment.
Thanks for the info.
Owner
|
@linesight Thank you for your work on this PR. I added a few comments, we're almost good to merge. I can accept a separate PR for documentation changes. Are all supported examples working fine? |
… is using complex iframe which is not good with cefpython javascript binding
Contributor
Author
I have checked all example python scripts, including those snippets, they are good to run, with below limitation -
so I am not able to confidently say they are good. Given my intent is windows focus, gtk isn't a popular GUI framework at windows, I would like to defer gtk investigation until cefpython-on-linux starts |
Owner
|
That's okay. As I understand this PR is for Windows support. |
Closed
Owner
|
We should also updated Build Instructions: https://github.com/cztomczak/cefpython/blob/master/docs/Build-instructions.md |
This was referenced Feb 18, 2025
Owner
|
This PR was reverted. Please send changes to |
Owner
|
@linesight Also please squash your commits into one big commit with a title like "Update CEF to version 123.0.7 with Chromium-123.0.6312.46 (Windows tested)". If you don't squash, I can squash it and merge, but then I don't know how you will be credited in the GitHub Contributors view (Insights tab). |
Owner
|
Also documentation should be in one PR or another PR, but let's not mix in both PRs. I can merge when I have an opportunity to test on a Windows machine. |
Contributor
Author
|
As there has been multiple dicussions and multiple git commits at my branch to address each feedback, it is way too much effort to squash my commit at my branch on my side. I want my branch to preserve my original git commit history. It is better you squash it during merge. Or you can configure the pull request setting at your depot to allow squash merging. |
This was referenced Feb 19, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added cef 123 support for python 3.10 and 3.11 at windows
Some of the fix is coming from https://github.com/llcc01/cefpython
qt.py example added pyqt6 support
addressing #652