current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite
1

I have made an application to display images in the form of gallery, from terminal if you are standing at the same location then it will open those images in the gallery if it does not have it will prompt to select the directory.

below is the module: gallery.py

import sys
import os
import utils

from PyQt4 import QtGui, QtCore

class MyListModel(QtCore.QAbstractTableModel): 
    def __init__(self, window, datain, col, thumbRes, parent=None): 
        """ Methods in this class sets up data/images to be
            visible in the table.
            Args:
                datain(list): 2D list where each item is a row
                col(int): number of columns to show in table
                thumbRes(tuple): resolution of the thumbnail 
        """
        QtCore.QAbstractListModel.__init__(self, parent) 
        self._slideShowWin = window
        self._thumbRes = thumbRes
        self._listdata = datain
        self.pixmap_cache = {}
        self._col = col

    def colData(self, section, orientation, role):
        if role == QtCore.Qt.DisplayRole:
            return None

    def headerData(self, section, orientation, role):
        if role == QtCore.Qt.DisplayRole:
            if orientation in [QtCore.Qt.Vertical, QtCore.Qt.Horizontal]:
                return None

    def rowCount(self, parent=QtCore.QModelIndex()): 
        return len(self._listdata) 

    def columnCount(self, parent):
        return self._col

    def data(self, index, role):
        """ method sets the data/images to visible in table
        """
        if index.isValid() and role == QtCore.Qt.SizeHintRole:
            return  QtCore.QSize(*self._thumbRes)

        if index.isValid() and role == QtCore.Qt.TextAlignmentRole:
            return QtCore.Qt.AlignCenter

        if index.isValid() and role == QtCore.Qt.EditRole:
            row = index.row()
            column = index.column()
            try:
                fileName = os.path.split(self._listdata[row][column])[-1]
            except IndexError:
                return
            return fileName

        if index.isValid() and role == QtCore.Qt.ToolTipRole:
            row = index.row()
            column = index.column()
            try:
                fileName = os.path.split(self._listdata[row][column])[-1]
            except IndexError:
                return
            exifData = "\n".join(list(utils.getExifData((self._listdata[row][column]))))
            return QtCore.QString(exifData if exifData else fileName)

        if index.isValid() and role == QtCore.Qt.DecorationRole:
            row = index.row()
            column = index.column()
            try:
                value = self._listdata[row][column]
            except IndexError:
                return

            pixmap = None
            # value is image path as key
            if self.pixmap_cache.has_key(value) == False:
                pixmap=utils.generatePixmap(value)
                self.pixmap_cache[value] =  pixmap
            else:
                pixmap = self.pixmap_cache[value]
            return QtGui.QImage(pixmap).scaled(self._thumbRes[0],self._thumbRes[1], 
                QtCore.Qt.KeepAspectRatio)

    def flags(self, index):
        return QtCore.Qt.ItemIsEditable | QtCore.Qt.ItemIsEnabled | QtCore.Qt.ItemIsSelectable

    def setData(self, index, value, role=QtCore.Qt.EditRole):
        if role == QtCore.Qt.EditRole:
            row = index.row()
            column = index.column()
            try:
                newName = os.path.join(str(os.path.split(self._listdata[row][column])[0]), str(value.toString()))
            except IndexError:
                return
            utils._renameFile(self._listdata[row][column], newName)
            self._listdata[row][column] = newName
            self.dataChanged.emit(index, index)
            return True
        return False

class GalleryUi(QtGui.QTableView):
    """ Class contains the methods that forms the
        UI of Image galery
    """
    def __init__(self, window, imgagesPathLst=None):
        super(GalleryUi, self).__init__()
        self._slideShowWin = window
        self.__sw = QtGui.QDesktopWidget().screenGeometry(self).width()
        self.__sh = QtGui.QDesktopWidget().screenGeometry(self).height()
        self.__animRate = 1200
        self.setUpWindow(imgagesPathLst)

    def setUpWindow(self, images=None):
        """ method to setup window frameless and fullscreen,
            setting up thumbnaul size and animation rate
        """
        if not images:
            path = utils._browseDir("Select the directory that contains images")
            images = slideShowBase.ingestData(path)
        thumbWidth = 200
        thumbheight = thumbWidth + 20
        self.setWindowFlags(
            QtCore.Qt.Widget |
             QtCore.Qt.FramelessWindowHint | 
             QtCore.Qt.X11BypassWindowManagerHint
             )
        col = self.__sw/thumbWidth 
        self._twoDLst = utils.convertToTwoDList(images, col)
        self.setGeometry(0, 0, self.__sw, self.__sh)
        self.showFullScreen()
        self.setColumnWidth(thumbWidth, thumbheight)
        self._lm = MyListModel(self._slideShowWin, self._twoDLst, col, (thumbWidth, thumbheight), self)
        self.setShowGrid(False)
        self.setWordWrap(True)
        self.setModel(self._lm)
        self.resizeColumnsToContents()
        self.resizeRowsToContents()
        self.selectionModel().selectionChanged.connect(self.selChanged)

    def selChanged(self):
        if self._slideShowWin:
            row = self.selectionModel().currentIndex().row()
            column = self.selectionModel().currentIndex().column()
            # if specific image is selected the slideshow opens paused.
            self._slideShowWin.playPause()
            self._slideShowWin.showImageByPath(self._twoDLst[row][column])


    def animateUpSlideShow(self):
        """ animate the slideshow window back up
            to view mode and starts the slideShowBase
            where it was paused.
        """
        self.animateUpGallery()
        self.animation = QtCore.QPropertyAnimation(self._slideShowWin, "geometry")
        self.animation.setDuration(self.__animRate);
        self.animation.setStartValue(QtCore.QRect(0, self.__sh, self.__sw, self.__sh))
        self.animation.setEndValue(QtCore.QRect(0, 0, self.__sw, self.__sh))
        self.animation.start()
        self._slideShowWin.activateWindow()
        self._slideShowWin.raise_()
        self._slideShowWin.playPause()


    def animateUpGallery(self):
        """ animate the gallery window up to make slideshow visible
        """
        self.animGallery = QtCore.QPropertyAnimation(self, "geometry")
        self.animGallery.setDuration(self.__animRate);
        self.animGallery.setStartValue(QtCore.QRect(0, 0, self.__sw, self.__sh))
        self.animGallery.setEndValue(QtCore.QRect(0, -(self.__sh), self.__sw, self.__sh))
        self.animGallery.start()

    def keyPressEvent(self, keyevent):
        """ Capture key to exit, next image, previous image,
            on Escape , Key Right and key left respectively.
        """
        event = keyevent.key()
        if event == QtCore.Qt.Key_Escape:
            if self._slideShowWin:
                self._slideShowWin.close()
            self.close()
        if event == QtCore.Qt.Key_Up:
            if self._slideShowWin:
                self.animateUpSlideShow()

def main(imgLst=None):
    """ method to start gallery standalone
    """
    app = QtGui.QApplication(sys.argv)
    window =  GalleryUi(None, imgLst)
    window.raise_()
    sys.exit(app.exec_())

if __name__ == '__main__':
    curntPath = os.getcwd()
    if len(sys.argv) > 1:
        curntPath = sys.argv[1:]
    main(utils.ingestData(curntPath))

and dependent module utils.py some of the methods in the below are used by module slideshow which can also launch the image gallery from which a 2D list containing image paths.

import os
import sys
import exifread
from PyQt4 import QtGui

def isExtensionSupported(filename):
    """ Supported extensions viewable in SlideShow
    """
    reader = QtGui.QImageReader()
    ALLOWABLE = [str(each) for each in reader.supportedImageFormats()]
    return filename.lower()[-3:] in ALLOWABLE

def imageFilePaths(paths):
    imagesWithPath = []
    for _path in paths:
        dirContent = getDirContent(_path)
        for each in dirContent:
            selFile = os.path.join(_path, each)
            if ifFilePathExists(selFile) and isExtensionSupported(selFile):
                imagesWithPath.append(selFile)
    return list(set(imagesWithPath))

def ifFilePathExists(selFile):
    return os.path.isfile(selFile)

def getDirContent(path):
    try:
        return os.listdir(path)
    except OSError:
        raise OSError("Provided path '%s' doesn't exists." % path)

def getExifData(filePath):
    """ Gets exif data from image
    """
    try:
        f = open(filePath, 'rb')
    except OSError:
        return
    tags = exifread.process_file(f)
    exifData = {}
    if tags:
        for tag, data in tags.iteritems():
            if tag != 'EXIF Tag 0x9009':
                yield "%s: %s" % (tag, data)

def convertToTwoDList(l, n):
    """ Method to convert a list to two
        dimensional list for QTableView
    """
    return [l[i:i+n] for i in range(0, len(l), n)]

def _renameFile(fileToRename, newName):
    """ method to rename a image name when double click
    """
    try:
        os.rename(str(fileToRename), newName)
    except Exception, err:
        print err

def _browseDir(label):
    """ method to browse path you want to
        view in gallery
    """
    selectedDir = str(QtGui.QFileDialog.getExistingDirectory(None, 
            label,
            os.getcwd()))
    if selectedDir:
        return selectedDir
    else:
        sys.exit()

def generatePixmap(value):
    """ generates a pixmap if already not incache
    """
    pixmap=QtGui.QPixmap()
    pixmap.load(value)
    return pixmap

def ingestData(paths):
    """ This method is used to create a list containing
        images path to slideshow.
    """
    if isinstance(paths, list):
        imgLst = imageFilePaths(paths)

    elif isinstance(paths, str):
        imgLst =  imageFilePaths([paths])
    else:
        print "You can either enter a list of paths or single path"
    return imgLst

I need a code review for the implementation, any Suggestions/hints is welcome that I can use to improve the code and more testable?

share|improve this question
    
MyListModel.colData() and MyListData.headerData() don't do anything. They always return None. –  Marc Poulin Dec 17 '13 at 5:17
    
if i don't set it to return none the header will show numbers in for rows and columns –  Sàn Luthra Dec 17 '13 at 15:22

4 Answers 4

up vote 1 down vote accepted

Disclaimer: this is not a design pattern review, but rather a Pythonization one.

  1. Not always returning a value: MyListModel.colData() MyListModel.headerData(). Even though Python implicitly returns None if you don't specify a return value or return statement at all, it is better to return explicitly in all the code branches, or have a catch-all return:

    def colData(self, section, orientation, role):
    if role == QtCore.Qt.DisplayRole:
        return None
    return None

  2. Inconsistent return statements: in MyListModel.data(), MyListModel.setData() and others you mix return statements with value and the ones without value. Consider returning None explicitly.

  3. In MyListModel.data() the check index.isValid() is repeated over and over again, a single check would be sufficient:

    if not index.isValid():
   return None

if role == QtCore.Qt.SizeHintRole:
    return  QtCore.QSize(*self._thumbRes)

  4. Always use a file as a context manager, this way you wouldn't leak open files (e.g. in getExifData), even if your code with throw during file processing:

    try:
    with open(filePath, 'rb') as f:
        # Work with file
except OSError:
    return

  5. Use list comprehensions and generator expressions, they rock! Here's how I would rewrite getExifData:

    def get_exif_data(file_path):
    """ Gets exif data from image
    """
    try:
        with open(file_path, 'rb') as f:
            return ("%s: %s" % (tag, data)
                    for tag, data in exifread.process_file(f).iteritems()
                    if tag != 'EXIF Tag 0x9009')
    except OSError:
        return

    Here's the imageFilePaths:

    def image_file_paths(paths):
    images = {
        joined_path
        for path in paths
        for each in get_dir_content(path)
        for joined_path in [os.path.join(path, each)]
        if if_file_path_exists(joined_path)
        if is_extension_supported(joined_path)
    }

    return list(images)

  6. You should really follow pep8, name your methods with_underscore_separators, but do notCamelCase them.

  7. Methods/functions should not have unexpected side-effects like printing error messages on their own (see _renameFile, ingestData) or even worse - perform exit (see _browseDir). Consider raising appropriate exceptions, e.g. ValueError would be a perfect fit for parameter type-checking.

share|improve this answer
    
great thanks @Ihor Kaharlichenko. I have habit of coding from my workplace where we do not use underscore within the method names, however camelCase sticking the rest with PEP8 I agree using context manager, intact earlier i was going use it, it just slipped my mind but i was returning generator object in that method. lastly for imageFilePaths method i wanted to keep the code more readable, for someone else the way you have accomplished a list being return could be slightly less readable. –  Sàn Luthra Dec 16 '13 at 16:07
    
expected comments on animateUpGallery and animateUpSlideShow in gallery.py module. –  Sàn Luthra Dec 16 '13 at 16:13
up vote -1 down vote 
    def ingestData(paths):
        """ This method is used to create a list containing
            images path to slideshow.
        """
        if isinstance(paths, list):
            imgLst = imageFilePaths(paths)

        elif isinstance(paths, str):
            imgLst =  imageFilePaths([paths])
        else:
            print "You can either enter a list of paths or single path"
        return imgLst

The imgLst variable is not set in all cases.

share|improve this answer
    
do you mean I have ti first initialize it as None or empty list ? –  Sàn Luthra Dec 17 '13 at 1:57
    
how is imgLst not set? –  Sàn Luthra Dec 17 '13 at 2:08
1  
When the "else" is executed, imgLst is not set. The statement "return imgLst" will fail. –  Marc Poulin Dec 17 '13 at 2:25
    
that is true. I will fix that , I am thinking to use an Exception instead of printing –  Sàn Luthra Dec 17 '13 at 4:31
    
I think this whole function can go away. Calling imageFilePaths(["filename"]) and making the user type the [ ] isn't really a burden in my opinion. –  Marc Poulin Dec 17 '13 at 5:10
up vote 1 down vote

I'm not sure this function adds any value.

def ifFilePathExists(selFile):
    return os.path.isfile(selFile)

Why not call os.path.isfile() directly?

share|improve this answer
    
then I need to know how to mock built-in os.path.isfile !!! since I am testing imageFilePaths method. –  Sàn Luthra Dec 17 '13 at 1:55
    
here is the [test][1] [1]: github.com/sanfx/slideShow/blob/master/tests/test_utils.py –  Sàn Luthra Dec 17 '13 at 2:02
    
I think I would create a test directory with some test files in it. No need to mock the isfile() function. –  Marc Poulin Dec 17 '13 at 2:26
up vote 2 down vote

I would change

    def ifFilePathExists(selFile):

to

    def file_path_exists(sel_file):

Saying "if if" feels wrong:

    if ifFilePathExists(filename):
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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