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

Removed considerations for Windows 95/98/ME #2400

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Oct 13, 2024

Please review this carefully, I'm a bit out of my depth in certain places and am left guessing. I've left GitHub comments on specific files.

@Avasam Avasam force-pushed the Remove-Windows-95-98-ME branch from c06e77c to 2935175 Compare October 13, 2024 21:45
Comment on lines 43 to 48
// Global\\ etc goodness:
// On NT4/9x, 'Global\\' is not understood and will fail.
// On 2k/XP, anyone can create 'global' objects.
// On Vista, you need elavated perms to create global objects - however, once
// On Vista, you need elevated perms to create global objects - however, once
// it has been created and permissions adjusted, a user with normal
// permissions can open these global objects.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to #2399

Comment on lines +168 to +169
// Do a LoadLibrary, as the Ex version may not always exist on Win95.
// TODO: We no longer support Windows 95, how should this code be updated ?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer support Windows 95, how should this code be updated ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment can just be removed - there's no LoadLibrary done here at all, right? But if there was, the comment is implying we could now use LoadLibraryEx if there was a good reason to do so.

Comment on lines +66 to +68
# in order to *use* the username/password of a particular dun entry,
# one must explicitly get those params under Win95 ...
# TODO: We no longer support Windows 95, how should this code be updated ?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer support Windows 95, how should this code be updated ?

Comment on lines +257 to -258
| win32con.MIIM_TYPE
)
# Note: No MIIM_TYPE - this screws win2k/98.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be tested, I have no idea of its actual effect. Just following the comment.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be safer to not add this constant, but leave a comment which notes it wasn't added due to those old windows?

Comment on lines +107 to +109
// On platforms that don't have NT security,
// the initialization of the SECURITY_DESCRIPTOR should fail,
// leaving the sd NULL.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if I should just essentially revert b9e3c3f

Or if there's valid use-cases for all the checks added there (CC @trentm @rupole )

@Avasam Avasam requested a review from mhammond October 18, 2024 18:50
Comment on lines +168 to +169
// Do a LoadLibrary, as the Ex version may not always exist on Win95.
// TODO: We no longer support Windows 95, how should this code be updated ?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment can just be removed - there's no LoadLibrary done here at all, right? But if there was, the comment is implying we could now use LoadLibraryEx if there was a good reason to do so.

Comment on lines +257 to -258
| win32con.MIIM_TYPE
)
# Note: No MIIM_TYPE - this screws win2k/98.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be safer to not add this constant, but leave a comment which notes it wasn't added due to those old windows?

@@ -467,7 +467,7 @@ def RegisterShellInfo(searchPaths):
core path, you can avoid packages re-registering the same path.
-m filename -- Find and register the specific file name as a module.
Do not include a path on the filename!
--shell -- Register everything with the Win95/NT shell.
--shell -- Register everything with the NT shell.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably just say "Windows shell"?

return PyErr_Format(PyExc_RuntimeError, "This version of Windows does not support this function");
}
PFNGetClipboardSequenceNumber pfnGetClipboardSequenceNumber =
(PFNGetClipboardSequenceNumber)GetProcAddress(hmod, "GetClipboardSequenceNumber");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A null check still makes sense to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants