My random shitty opinion, don’t take it personally, I didn’t slept well, also I’m late for work:

README: you use both py and python3, choose one because I’m confused! Also you say “Navigate to ./src/” No, I’m a lazy user and I want instructions that I can copy-paste, it’s always better when you clone a random project (especially at work) and be able to copy-paste, like:

1. Install Python 3 (and NOT “make sure…” it’s confusing, how would I make sure? Your code could be used by non-programmers you know? also put a link to python.org too)
2. Run:

py -m pip install -r requirements.txt
cd src/
py main.py

3. Do this
4. Do that
5. (…)

Be affirmative! Also “This will install pip” could be wrong on most systems, remove that sentence if not true.

Still in the README, why should I run the thing from src? Is your application broken if I I do “py src/main.py”? What happens?

It seems like the GUI and the code that watermarks are mixed and that’s annoying. If it was clearly separated, you could make a command-line versions of your application in 5 minutes without changing the GUI, for example with argparse.

Why is there so much code to set the layout in main.py? Put that stuff in Layout, I don’t want to see that in my main. Also do “def main(): …” and “if __name__ == ‘__main__’” or something, it’s cleaner, and it prevents errors if I “import main”

Do you really really need all those members variables? I understand that Tk is weird, but ImageManager has 12 members, main has 3 instead of 1 (the main “Window”), and Layout has a billion members. For example total_columns and total_rows are not used in Layout.py, that’s confusing. ImageManager.SAVE_DIR and IMAGE_RESIZE_FACTOR are constants, move them out. DEFAULT_FOLDER is only used once, merge it with TEST_BG, that kind of thing.

ImageManager.path_is_valid is useless and potentially harmful because you’re duplicating standard code, remove it and use path.exists, no need to replicate the standard library, and other coders know what’s inside the path module, they don’t know what’s in your code and why it’s different from the standard modules because they’ll think “if it’s there, it must do something special!” (but it’s not special at all here)

Ideally you shouldn’t put testing code in your main code. TEST_BG and TEST_FG will have to be removed. I understand why it’s there, it’s faster for your test, but it always show that the architecture has flaws. For example here, it shows that you forgot to make it possible to load those things on the command line, like main.py --test-bg space.png --test-fg python-watermark.png or better main.py --bg space.png --fg python-watermark.png, see? You have the beginning of a command-line application!

On GitHub you have 6 branches, that’s madness. Merge them all one at a time, or delete them. Too many experiments are not good.

You commit messages are good and expressive, that’s nice! Also I see that you used the standard .gitignore for Python on GitHub, that very nice and way better than doing one from scratch who will miss a lot of stuff.

I’ll come back later if I can.

Edit: there is hardcoded paths “/home/mike/code” and no default pictures, I can’t test it right now, that’s something to fix too.

def path_is_valid(path: Path) -> bool:
    if not path.exists():
        return False
    return True

There’s no reason for this function to exist. Can you see why?

If I was gonna make a suggestion, it would be to use some formatting tool such as black to make sure your code is styled in a standard way.

I can give you some basic Python set-up advice (which is hard-won because nobody tells you this stuff):

1. Use pyproject.toml, not requirements.txt. It’s better and it’s the recommended way.
2. Always use type annotations. I saw you used it a bit - well done! But just use it everywhere. Add declarations & type annotations for your class member variables. You’ll thank me later! Also prefer Pyright to Mypy; it is far far better (and the default for VSCode).
3. I would recommend using Black to autoformat your code. It’s easily the best Python formatter. You can automate it using pre-commit.
4. ALWAYS PUT MAIN IN A FUNCTION. You should not run any code at a global scope. Do it like this:

def main():
  ...

if __name__ == "__main__":
    main()

There are two reasons:

1. Generally it’s good to keep variables to the smallest scope possible. Putting everything in a global scope will just mean people start using it in their functions, and then you have a mess.
2. Any code you put in a global scope gets run when you import a module. Importing a module should not have any side effects.

Other minor things:

1. Path has a crazy/neat override of the / operator so you can do Path("foo") / "bar" / "baz.txt".
2. Don’t use assert for stuff that the user might reasonably do (like not choosing a background image). assert is meant to be for things that you know are true. You are asserting them. Like “hey Python, this is definitely the case”.
3. TK is an ancient shitty GUI toolkit. I would recommend QT instead (though it will probably be more pain to set up).

It’s Saturday night and I have nothing interesting to do, so I’m going to go through your code and leave a bunch of comments 'cause this is fun for me :-) Please don’t interpret the number of comments as a negative, there’s always something to criticise, even in work done by very experienced people. I just like to be thorough.

1. You’ve got a readme.md file, which is good, but convention is that it should be named README.md. I know, it’s weird, but that’s the convention.

2. You’ve pinned your dependency (Pillow) version, which is excellent, and at this stage you’re just dealing with the one dependency so it probably feels overkill, but you may want to look into more robust tools like Poetry for managing your dependencies. Poetry will handle all dependency management and upgrades and stuff those dependencies into a pyproject.toml file in the project root. Conveniently, you can use this same file to configure other tools you’ll come to depend on, like Ruff and Coverage. Some of the other comments here have recommended Black for formatting, but I think you’ll find that the community is moving away from the combination of linting & formatting tools (Black, iSort, flake8, pycodestyle) and are instead embracing “do everything” Ruff.

3. Python convention is that all files should be named with snake_case as opposed to your CamelCase. Python will still work however you name your files, but if you want to write your code such that it plays well with other developers, it’s good to conform.

4. Using a tool like Ruff or Black will ensure that your code conforms to community standards, again making it easier to read for everyone else. You’ve got some rather uncommon indenting and white space choices in your code that make it hard to read for those of us trained on the common style, so I recommend embracing one of these tools to conform your code.

5. Before reaching for environment variables, consider what you can get with Python built-ins and the standard library. For example, you don’t need to import os and then run environ["PWD"] to get the current directory. In fact, doing this is a Bad Idea since it assumes things like the OS and environment that you can’t always count on. Instead, try just calling the built-in pwd() or Path.cwd(). Similarly, if you want to reference your ${HOME} folder, you can use Path.home().

6. I love the use of classes. Not everyone does, but frankly I don’t understand people who avoid them. With that said though, you’ve implemented yours with behaviours you usually see in Java or C++, using getters & setters. Python will let you do this, but often it’s easier/cleaner to just allow calling code to set class properties directly. So for example, your .set_fg_position() method, could just as easily be done externally with my_manager.fg_x_position, my_manager.fg_y_position = coords

7. For that matter, it’s a good idea to define your methods to expect the format you want, rather than be “forgiving” and do the conversion for you. So the aforementioned set_fg_position() might be better like this:

   def set_fg_position(self, coords: tuple[int, int]) -> None:
       self.fg_x_position, self.fg_y_position = coords
   

   Force the calling code to conform to the interface. It keeps your code tidy and your interface predictable.

8. Don’t use assertions. In some projects, assertions are disabled in production and shouldn’t be counted on for validation or anything like that. They’re good for automated tests though.

9. For that matter, you should consider writing some tests. Python comes with a built-in unittest framework, so you can just subclass TestCase and go to town. If you like though, there’s also PyTest just please if you choose to use it, keep your tests simple and don’t overindulge in conftest.py. That way lies madness.

10. If your project depends on an asset like an image or font, you should include that asset in your source code. I see you’re referencing Pillow/Tests/fonts/FreeMono.ttf, but I’m not sure that you can count on that font being installed and available everywhere. Better to either have it in your project so you can reference it with a relative path, or list it as a dependency and talk about how you install it. I may be wrong on this though, given the Pillow in the path. I’m not sure how the font would be found if it’s in a virtualenv as part of the Pillow installation, but it’s weird.

11. You definitely don’t want to be using environment values like OLDPWD for something like a SAVE_DIR. That variable is just the directory you were in before you came to your current one, so it’ll likely be different every time you run the program! Instead, I strongly recommend you define these values to be constant. Something like . (current folder) or Path.home() / ".config" / "myproject".

12. For that matter, constants and variables are meant to be written differently in Pythonland. A constant should be in ALL_CAPS while a variable should be snake_case. I see you’re using that style, but you’re setting a constant on a class instance with self.SAVE_DIR = .... That’s pretty odd, as it’d mean that potentially your “constant” can have a different value between instantiations. What you probably want is a class constant, which is defined outside the __init__:

    class ImageManager:
        SAVE_DIR = pwd()
        IMAGE_RESIZE_FACTOR = 5
        def __init__(self) -> None:
            ...
    

13. Looking at Layout.py now, I see you’ve baked your personal computer’s directory structure into your program. This is a classic “it works on my machine” situation ;-) You probably should set make IMG_HOME a variable that’s user-defineable (maybe with a command line argument?) or if the files in question can/should be part of your project, you could do something like:

    IMG_HOME = Path(__file__).parent / "path/to/assets/imgs"
    

14. I love the use of type hints you’ve got going on, but they’re incomplete. It’s a good idea to strive for type completion if you can get it. It will help your IDE find bugs you didn’t realise were there, and if you wanna go all out and adopt mypy (use it with caution, it’s not a religion), it’ll help you get to complete type support.

15. Looking at main.py now, I note that you’ve made the classic mistake of executing code on module load. This is a big Python no-no, because it means that if someone were to import your code, it would trigger your program! The standard way to do this properly is with this wacky incantation:

    def main():
        # This is where the program lives
    
    
    if __name__ == "__main__":
        main()
    

16. The paths in this file are definitely going to break on other people’s computers as well. /home/mike/bg does not exist on my machine ;-) Similarly, you’ve used Path(Path(os.getcwd()).parent, "assets", "img") which will only work if (a) the current working directory has assets/img/ in it, and (b) assets/img actually exists on the machine. As they’re not in this repo, there’s no way this would run for me :-(

You probably want to (a) copy the assets required for the project into the repo, and (b) define values like DEFAULT_FOLDER and WATERMARK_DIR based on values you know will exist on everyone’s machines, namely either (a) relative to your project (using the Path(__file__).parent() trick I mentioned above, or (b) somewhere you’ve instructed the user to place them as part of the installation.

Finally, kudos on your choice of license. We need more AGPL code in the world <3!