- 
                Notifications
    
You must be signed in to change notification settings  - Fork 98
 
customicon support #96
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
base: master
Are you sure you want to change the base?
Conversation
| return self._set_string_field('Notes', value) | ||
| 
               | 
          ||
| @property | ||
| def icon(self): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the icon getter and setters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the exact same functions exist in the baseelement; didn't seem necessary
| ) | ||
| self._version = version | ||
| 
               | 
          ||
| self._meta = meta | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: what's this meta stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The KDBX XML contains a Meta tag which contains the customicons (the actual image files). Since it doesn't use a generic key index but instead a base64 index to link the image to the entry, I send the meta tag + children to the the entry, so the base64 index can be converted to a numerical one (as the keepass UI does as well)
| entry.expires = False | ||
| entry.expiry_time = changed_time | ||
| entry.icon = icons.GLOBE | ||
| entry.customicon = "9" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this even be valid? I was expecting more something like an actual image file
EDIT: And icon / customicon should have their own dedicated test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way keepass works is that when you set a customicon, it sets the default icon to 0. If you do not want to use the customicon anymore, the CustomIconUUID tag should be removed from the entry. I can only create a separate test for customicons if I modify the test KDBX files; the code will not set customicon to a value which does not actually exist. So setting this to 9 will only result in a customicon with index 9 if at least 10 customicons exist. In all other cases (as it does here) customicon is set to None, which removes the tag from the entry XML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to elaborate, customicons are only stored in the database once and are referred to by entries by using a UUID which is base64 encoded. This PR does not add the possibility to add actual image files as custom icons, but merely allows you to set customicons which already exist in the DB by providing their numerical index.
| def tearDown(self): | ||
| os.remove(os.path.join(base_dir, 'change_creds.kdbx')) | ||
| 
               | 
          ||
| class CtxManagerTests(unittest.TestCase): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this was intentional, but probably happened because you merged some pull requests after I rebased; I guess it can be ignored.
        
          
                README.rst
              
                Outdated
          
        
      | # save database | ||
| >>> kp.save() | ||
| Context Manager Example | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you deleting this?
| # add a new entry to the social group with a custom icon | ||
| >>> group = find_groups(name='social', first=True) | ||
| >>> entry = kp.add_entry(group, 'testing', 'foo_user', 'passw0rd') | ||
| >>> entry = kp.add_entry(group, 'testing', 'foo_user', 'passw0rd', customicon="2") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use single quotes
| 
           We should implement this fully for entries and groups before merging.  | 
    
cf4549b    to
    a6dd83d      
    Compare
  
    | 
           I would also really like custom icons. I see it's long time this PR is pending now...  | 
    
| 
           Is there a chance to get customicons for groups and entries?  | 
    
2a37542    to
    3c1af14      
    Compare
  
    
Updated to the latest master and reworked some parts to also support customicons for groups.