2 changes:
-Shortcut <Control>D for deleting a current bookmarked page, Shows a confirmation dialog.
-Bookmarked files thumbnails are now with a Bookmark icon to indicates that page is bookmarked
I quite like the idea of putting a bookmark icon on thumbnails of pages that are bookmarked. Nice.
Now, for a few things that I noticed while reading over the patch file:
- The message dialog in bookmark_backend.py should probably be imported from mcomix.message_dialog instead, since this one comes with functionality to disable the message if desired, e.g. set_should_remember_choice('bookmark-remove-current', (gtk.RESPONSE_OK,))
- Adding the image to icons.py's list isn't necessary, as it isn't used as GTK+ resource, from what I can tell.
- Speaking of the icon, you should probably attach it to this issue as well. (the diff file doesn't contain binary changes), with some indicator where it was taken from and under which license it was released.
- In thumbbar.py, the image file can be loaded from a relative path by calling image_tools.load_pixbuf_data. You can compare icons.py, lines 61-62 for that.
That's pretty much it. Good work. One final note, which isn't a code problem per se: Having comments containing only the code's author isn't really necessary, as it somewhat fragments the code. That is what svn blame is for. ;)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thank you for the feedback. There were a huge holliday here in Brazil so i went to my hometown for some time, sorry i couldn't answer you before. I'll take a look in all of this. Thank you again :]
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
For me this is a regression.
I intentionally do not want to be prompted every time I remove a bookmark*.
There are two issues here
(1) prompts are annoying if you are doing something simple over and over again (get out of the way of the user)
(2) accidentally deleting something is annoying (programs should forgive user mistakes)
(1) I like to make lots of bookmarks as I read and later delete lots and lots of bookmarks.
(2) prompting before delete hides the real problem. In Mozilla you can delete a bookmark and undo the change. fixing it fully is harder though.
* Further explanation:
Older versions of MComix included a menu item "Clear bookmarks" and understandably, you would want to warn users before permanently clearing and removing all bookmarks, that is a big change and not something you'd want to hit accidentally (2). I removed this item because I wanted the extra menu space for bookmarks, and because removing all the bookmarks was not a behaviour I would expect anyone to want to do very often, and it is inconsistent with how other programs do Bookmarks.
I figured that since you can only remove a bookmark in the Edit dialog the chances of doing it accidentaly were quite low and a prompt was unnecessary. Again being able to undo a delete an accidentally delete would be the ideal fix and consistent with how other programs do bookmarks.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Patch file
Plz, feel free to give any feedback/critics/advice. ; )
I quite like the idea of putting a bookmark icon on thumbnails of pages that are bookmarked. Nice.
Now, for a few things that I noticed while reading over the patch file:
- The message dialog in bookmark_backend.py should probably be imported from mcomix.message_dialog instead, since this one comes with functionality to disable the message if desired, e.g. set_should_remember_choice('bookmark-remove-current', (gtk.RESPONSE_OK,))
- Adding the image to icons.py's list isn't necessary, as it isn't used as GTK+ resource, from what I can tell.
- Speaking of the icon, you should probably attach it to this issue as well. (the diff file doesn't contain binary changes), with some indicator where it was taken from and under which license it was released.
- In thumbbar.py, the image file can be loaded from a relative path by calling image_tools.load_pixbuf_data. You can compare icons.py, lines 61-62 for that.
That's pretty much it. Good work. One final note, which isn't a code problem per se: Having comments containing only the code's author isn't really necessary, as it somewhat fragments the code. That is what svn blame is for. ;)
Thank you for the feedback. There were a huge holliday here in Brazil so i went to my hometown for some time, sorry i couldn't answer you before. I'll take a look in all of this. Thank you again :]
For me this is a regression.
I intentionally do not want to be prompted every time I remove a bookmark*.
There are two issues here
(1) prompts are annoying if you are doing something simple over and over again (get out of the way of the user)
(2) accidentally deleting something is annoying (programs should forgive user mistakes)
(1) I like to make lots of bookmarks as I read and later delete lots and lots of bookmarks.
(2) prompting before delete hides the real problem. In Mozilla you can delete a bookmark and undo the change. fixing it fully is harder though.
* Further explanation:
Older versions of MComix included a menu item "Clear bookmarks" and understandably, you would want to warn users before permanently clearing and removing all bookmarks, that is a big change and not something you'd want to hit accidentally (2). I removed this item because I wanted the extra menu space for bookmarks, and because removing all the bookmarks was not a behaviour I would expect anyone to want to do very often, and it is inconsistent with how other programs do Bookmarks.
I figured that since you can only remove a bookmark in the Edit dialog the chances of doing it accidentaly were quite low and a prompt was unnecessary. Again being able to undo a delete an accidentally delete would be the ideal fix and consistent with how other programs do bookmarks.
Also the choice of shortcut is confusing, in Mozilla <Control>D is the shortcut for Add Bookmark.
I should say I do like the idea of marking the thumbnails to indicate a page is bookmarked, it is only smaller details I am asking about.