2
\$\begingroup\$

Intro:

Hello, I'm a python beginner. I haven't had anyone use my code or anything. I want to improve and become a "professional python developer". I thank you for taking the time to help me along that path.

Question:

In context of this forum my specific question is; regarding my codes clarity of purpose. Does my code clearly demonstrate it's use, requirements, and results? If you were handed this in a package to update or maintain would you need to reference an expanded help doc?

Regardless of the answer, if you feel you have useful feedback on improving the readability of the code please let me know. Thank you very much.


The blog link: https://valtyrtriit.blogspot.com/2021/12/python-wallpaper-scroller-entry-1-rough.html
Blog has a video demonstration of the code in action.


Optional Assistance:

  1. looking for a code portfolio review. I was told I should start a code blog with some python projects for 'other-eyes' to see. I'm the only one reading my code so I have no idea what it's quality is. I would appreciate any advice on the blog format / presentation / readability / topic / credits what have you.

Pre-Post Realizations:

As it does while considering the presentation of my work. I realize the unpack_gif() also stores, gets the gif_fps, and plays the image as well. This could be three different functions to isolate responsibility.


The source code:

import os # To create folders and save gif frames
import ctypes # To set the windows background image
import glob # To pull file contents of a directory
from PIL import Image # To parse gif data


def import_images( direct:str, fext: [ str ] ) -> { str : [ str ] }:
    """Takes a directory and a list of files of image type to include. Returns a dictionary with the file extension as a key and a array of image names."""

    """Prime the dictionary with the { key(extension) : value(empty array) } """
    img_dict = { }
    for extension in fext:
        img_dict.update( { extension : [] } )
    
    """Use glob() to pull in all files of extension at path, clean the name string and append it to appropriate key : array."""
    for extension in fext:
        for filename in glob.glob( f'{ direct }\\*.{ extension }' ):
            file = filename.replace( f"{ direct }\\", "" )
            img_dict[extension].append( file )
    return img_dict


def set_background( path:str ) -> None:
    """Takes in a absolute path and call the OS to set background image."""
    ctypes.windll.user32.SystemParametersInfoW(
            20, # System wallpaper index
            0, # Buffer
            f"{ path }", # File Absolute Path
            0 # Final Buffer
        )


def unpack_gif( directory:str, image:str, temp:str="temp_frames" ) -> None:
    """Takes in a directory path, an image name.gif, and a new temp folder name. Then parses out the gif's frames into the temp folder at directory. Once a valid path is available set that background to path."""

    image_path = os.path.join(directory, image)
    with Image.open( image_path ) as im: # Open the gif with pillow

        temp_folder = temp # Name of temporary gif frames folder
        folder_path = os.path.join(directory, temp_folder)
        try:
            os.mkdir( folder_path ) # Create the temporary folder to hold the gifs frames
        except: pass
        
        frame_duration = im.info['duration'] # Get gif run length in milliseconds
        img_fps = ( ( frame_duration / im.n_frames ) / 1000 ) # Get the time per frame in milliseconds and convert it to seconds.

        for frame in range(im.n_frames): # Get the number of frames in the gif
            im.seek( frame ) # The current frame in for loop 0 - end_frame
            name = f'temp_gif_{frame}.png' # Create a name for the frame image
            path = os.path.join(folder_path, name)
            
            im.save( path, 'GIF' ) # Save the frame image to the temporary folder
            set_background( path ) # Now we have a path to set the background, call the set function
            time.sleep( img_fps * 10 ) # slow down to not congest the wallpaper assignment
\$\endgroup\$
1
  • 2
    \$\begingroup\$ You should read PEP8. The whitespaces after parentheses and the missing whitespace after colons and around = on type-hinted kwargs look amazingly weird. \$\endgroup\$ Dec 30, 2021 at 8:51

1 Answer 1

1
\$\begingroup\$

First of all, good work! It looks like a fun project. I was able to understand the basics of how the program works from a quick read-through, and I think it is generally very clearly structured and the comments relevant and clear.

There are quite a few things that can be improved with regard to style, though.

I did a quick review of your code and made some notes as I went for your reference.


Notes

  1. No need to comment import statements. Readers of your code are familiar with the idea of importing and want to get straight into reading your code. Comment later at the point of using the external libraries if necessary.

  2. Refer to PEP8 (https://www.python.org/dev/peps/pep-0008/) and read other people's Python code for conventions around spacing in function definitions, etc.

  3. Wrap long typehints and comments to around 80 chars width in keeping with PEP8 and general best practice. You want your code to be tall and thin, not short and fat for better readability.

  4. It will be much easier to understand your code if variable names clearly describe what kind of information they hold(e.g. fext -> file_exts). Also avoid clashing with other English words. direct sounds like it has something to do with the adjective or verb 'direct', so use dir or directory. The ideal situation is that people can understand your code without having to read the comments!

  5. Put comments on a separate line (above the code) in general to avoid very long lines. Your can use same-line comments in specific situations like in your set_background function, but use tabs to align and make easier to read.


Edited code

Please compare my edited code to your original code and take away any points that you think might help you to write clearer code in the future. Note that I have not run this code, so you may need to do a little debugging if I made any typos or anything (although at a quick glance there were already a few errors in the code you originally posted?)

import os
import ctypes
import glob
from PIL import Image


# ====================
def import_images(dir:str, file_exts:[str]) -> {str:[str]}:
    """Takes a directory and a list of files of image type to include. Returns
    a dictionary with the file extension as a key and a array of image
    names."""

    """Prime the dictionary with the { key(extension) : value(empty array) }
    """
    img_dict = {}
    for extension in file_exts:
        img_dict.update({extension:[]})
    
    """Use glob() to pull in all files of extension at path, clean the name
    string and append it to appropriate key : array."""
    for extension in file_exts:
        for filename in glob.glob(f'{dir}\\*.{extension}'):
            file = filename.replace(f"{dir}\\", "")
            img_dict[extension].append(file)

    return img_dict


# ====================
def set_background( path:str ) -> None:
    """Takes in a absolute path and calls the OS to set background image."""

    ctypes.windll.user32.SystemParametersInfoW(
            20,         # System wallpaper index
            0,          # Buffer
            f"{path}",  # File Absolute Path
            0           # Final Buffer
        )


# ====================
def unpack_gif(directory:str, image:str, temp:str="temp_frames" ) -> None:
    """Takes in a directory path, an image name.gif, and a new temp folder
    name. Then parses out the gif's frames into the temp folder at directory.
    Once a valid path is available set that background to path."""

    image_path = os.path.join(directory, image)
    # Open the gif with pillow
    with Image.open(image_path) as im: 

        # Name of temporary gif frames folder
        temp_folder = temp 
        folder_path = os.path.join(directory, temp_folder)
        try:
            # Create the temporary folder to hold the gifs frames
            os.mkdir( folder_path ) 
        except: pass
        
        # Get gif run length in milliseconds
        frame_duration = im.info['duration'] 
        # Get the time per frame in milliseconds and convert it to seconds.
        img_fps = ((frame_duration / im.n_frames ) / 1000 )

        # Get the number of frames in the gif
        for frame in range(im.n_frames): 
            # The current frame in for loop 0 - end_frame
            im.seek(frame) 
            # Create a name for the frame image
            name = f'temp_gif_{frame}.png' 
            path = os.path.join(folder_path, name)

            # Save the frame image to the temporary folder
            im.save(path, 'GIF')
            # Now we have a path to set the background, call the set function
            set_background(path) 
            # slow down to not congest the wallpaper assignment
            time.sleep(img_fps * 10)
\$\endgroup\$
1
  • \$\begingroup\$ Thank you! I thought it was a fun mental break from my main project. Thank you for taking such time to write this response. Much appreciated! The script is much easier to read with the inline comments split out. and your other edits! @Richard N. also suggested reading PEP8, I took a look but it was a little difficult to understand. I"ll keep plugging away at it! I don't think there are any errors the script runs without raising anything. There were some logic bugs I worked out recently regarding gifs and mp4s (parsing and displaying mp4 is a new feature not posted here.) \$\endgroup\$
    – Valthalin
    Dec 31, 2021 at 14:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.