Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Plugins
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Version 3.2
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by motionman95 - 2009-05-03
Last edited by teru - 2009-12-14

FS#10187 - Theme Deleter - Parses CFG files and removes their files

This is plugin that will parse a given config (.cfg) file and remove it’s files in an attempt to “remove” the files installed by a theme, as requested by BdN3504 (on the rockbox forums). This is the plugin in it’s most basic/beta state, TONS of work must be done before it’s complete.

For now it relies on the “cabbiev2.cfg” file being placed in the same directory as it. Also, as pointed out in the forums (I forget by who) that it may not be a good idea to remove certain files, such as the font or iconset files. So, to sum it all up, this plugin is really just a starting point, and user input will definitely be a factor in it’s growth.

Closed by  teru
2009-12-14 05:51
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

committed in r23130.

The entries in CATEGORIES and SOURCES should follow alphabetical order, and the font doesn’t come as part of the theme and should not be deleted

Your patch doesn’t follow the coding style guidelines (spaces instead of tabs)

Hilton: there are several themes that *do* include a font. This is nothing special and has allways been that way – the only exception are the themes shipped with the rockbox builds and themes relying on the fonts package. The more important thing is that before deleting a file all other themes need to get checked if some other theme is using that file. Especially fonts, but other files might be affected as well. Also, I don’t see a reason thy CATEGORIES and SOURCES would need to be alphabetical – in fact, SOURCES is definitely *not* sorted alphabetically. Please don’t make such obvious wrong statements as that will only cause confusion.

@Dominik: They are alphabetical anyway.

Hilton: a somewhat random snippet from apps/plugins/SOURCES:

#ifdef HAVE_LCD_BITMAP /* Not for the Player */
rockblox1d.c
brickmania.c
maze.c
mazezam.c
text_editor.c
wavview.c
robotfindskitten.c

from my understanding of “alphabetical” brickmania.c has to come before rockblox1d.c if that would be sorted. Thus, they are NOT sorted alphabetical. Parts of the file are, but for compiling it doesn’t matter at all. You call that alphabetical?

can’t you use what i already wrote on the forum, to get the locations?
[…] in settings.h

#define WPS_DIR ROCKBOX_DIR “/wps” #define THEME_DIR ROCKBOX_DIR “/themes”

Yeah, but I’m not sure if there would be any point - seeing that it’s loaded from the CFG file…

teru commented on 2009-05-23 12:19

i wrote another plugin to remove theme.
this works as viewer and user can select to remove a file or not.
only tested on a simulator.

i compiled your patch with a check out from yesterday, but it seems corrupted. First there is no version info file in the zip file i created, then i had to execute “make zip” two times, because “make && make zip” did not give me a zip output file. The patched build runs well, but as i understood your approach, you made it as a viewer plugin for cfg files, right? but when i enter the context menu of a cfg file and select “delete theme”, only the cfg file is removed. another strange thing is, that when i tried to open a .txt file, it did not show up like with the daily build, but three splash messages are displayed on top of each other, the last one (which is the only one that is readable) says “done”. when i then enter the context menu for txt files, and choose “open with” i get only three options: search, sort, properties.
Something must have gone wrong, although i get no errors, when i compile.

teru commented on 2009-05-25 13:48

Maybe something was wrong. Could you try this one?
I tested both simulator and my gigabeat. version is r21072.
I forget to write how to use this plugin, sorry.
usage:
goto “Main Menu” → “Settings” → “Theme Settings” → “Browse Themes”.
enter Context Menu for the cfg file to remove, and execute theme_remove in “Open With…“.
selecting “Remove Theme” will remove wps, backdrop, and the cfg file. Font is not removed by default.

It works great for me

i compiled with r21090 and it works. I think you did it very nice. Including the theme remover in the cfg file context menu would mean you’d have to code it into the core, right? or would it be easily possible? that’s the only thing i’d like to improve. You did a really good job, thank you very much!

Thanks for the viewer, teru. I’d been really busy of late and had forgotten about this patch. But it seems that you’ve done a much better job! Glad to see that my work inspired you, if it did.

teru commented on 2009-05-27 13:01

Thanks for the feedbacks. I’m glad to hear it worked.
I think it’s not so difficult to add “Remove Theme” option to the context menu.
IIUC, “Properties” and “Add to Shortcuts” in the context menu use plugins, so i guess it can be done in the same way.

Needs resync

teru commented on 2009-07-21 14:30

sync and update patch.
*add option “Remove if not Used”.
*save options.
*create log file “/theme_remove_log.txt”.

also, attach a patch to add item to context menu.
for this, apply theme_remove_context_menu.patch with theme_remove.patch.
open context menu on .cfg file and “Remove Theme” will be shown in it.
selecting this is same as selecting theme_remove in “Open With…“.

This is fantastic!
i have compiled a sim with both patches and uploaded it to my rs account. if anybody wishes to see how this feature looks on target check it out!

http://rapidshare.com/files/258412449/PatchedGigaSimrar.zip

i hope this patch will make it into svn, cross your fingers!

teru commented on 2009-07-24 13:29

I think manual is needed to commit this patch…

OK.

One glitch i have observed: In the log file, if an item is not present it says: “Not exists”. This shall be “Does not exist”.

A suggestion for improvement: Attach some ifdefs for targets that don’t have remote control displays. For example i think it’s unneccessary to ask a gigabeat owner to remove the “remote wps” or the “remote viewers iconset” or the “remote iconset” because these things don’t exist on targets without a remote display.

My first attempt was a bit shitty. Here’s the proper version which also includes some fixes for that part of the manual.

final version, fixed two minor glitches.

teru commented on 2009-07-26 15:28

nice work. thank you!
i turned it to plugin manual and added ifdefs.
also, integrate almost same option descriptions.

I have done some minor corrections. Although i like the idea of having the plugin in the context menu like with your theme_remove_context_menu.patch much better. do you think it’s more feasible to implement it as a plugin?

The manual entry says “The changes done to the files could break another theme.” - can you expand on this?

I can think of the case where fonts from rockbox-fonts.zip are deleted by this plugin, and then themes installed at a later date which rely on that theme will stop working. Is there anything else?

I’m just slightly worried that if it’s not perfect, then this plugin could do more harm than good.

teru commented on 2009-07-27 16:14

improve a bit. now, there is no problem I know.
*use global_settings struct to check files currently used.
*add rockbox-fonts.config which contains filenames in rockbox-fonts.zip to prevent removing fnt files from the zip.

imo it would be possible to add context menu after plugin is committed, although i’m not sure if it is worth doing or safe enough.

One last thing:

Could you change the title of the menu from “Theme Remove” to “Remove [Theme Name]” ? (and maybe let it scroll, if the theme’s name is too long)

i attached a proper manual patch containing the directory locations.

Is the note in the manual still needed (about possibly breaking themes) ? According to Teruaki’s last post, there are no known issues.

I think if you choose always remove, then there is the possibility that other themes don’t work. but i am not sure. if that’S not the case, then we take that note out of the manual of course.

Shouldn’t this be moved to a new patch? I mean, it isn’t my patch anymore…at least not really.

i don’t understand… everything that has been posted here serves the purpose of the plugin mentioned in the task title. So what’s the significance of the name of the person who started this patch? i think there’s no need to move it to a new task. which purpose would that serve?
On topic: apparently i am too noob to post a proper patch. The very first line shall of course be

Index: manual/plugins/theme_remove.tex

and not

Index: theme_remove.tex

sorry, but i am still learning. plus i did svn add theme_remove.tex and then svn diff, because i thought that’s how it works, but apparently either it just doesn’t or i did something wrong.

teru commented on 2009-07-29 15:49

*change title of menu from “Theme Remove” to “Remove [themename]”.

I believe this plugin basically wont cause problem. but this plugin isn’t perfect and I have no idea what kind of things would happen.
maybe what we need is more tester?

BdN3504: maybe you added theme_remove.tex in wrong directory?

brilliant! i tested and it even scrolls! nice!

yes, i added it in the source… now i used svn diff correctly and have it right patch for the right files.
thanks to pixelma, i now know how to make a proper newline. before the note i could remove the descriiption tags in the first paragraph.

i just finished compiling patched versions of rb for all currently supported targets (see froums). what do you want to have tested exactly?

Player: Gigabeat F40
Bug: Go to “Menu/Settings/Theme Settings/While Playing Screen/” now go back to “Theme Settings” Then to go to /Browse Theme Files/” it says “Says “No Files!” but if you exit the menu and got to /Browse Theme Files/ FIRST it shows the files that are there.

cant edit previous comments???

*add “I am running Running Version: r22090M-090801”

teru commented on 2009-08-10 13:26

I think that is not a bug of the patch and is fixed in r22094.

alirght. guess ill wait for a new version on the forums. thats where i got the one im testing with

OK, so it was a dumb idea to patch a current unstable build. i have learned from my mistakes and have created a patch suitable for release 3.3.
The only thing i had to change were the strlcpy s to strncpy s. i have tested it on a sim and it doesn’t cause a problem, even if you have extremely long filenames.
So for testing purposes, i compiled five 3.3 versions and posted them to the forums. This time, i only compiled for targets which have actually been downloaded. if you want a testbuild for a specific target which isn’t in the forumthread, either contact me, or download and apply the patch i provide after you check out 3.3 using

svn co svn://svn.rockbox.org/rockbox/branches/v3_3 rockbox-3.3

The patch is still up to date. i had to resync the manual entry though. and psycho_maniac didn’t quite understand how the plugin works, so i added a reference to the entry.

would be be able to just hold select and get “delete theme” or would this be the only way of doing it this way?

if you use http://www.rockbox.org/tracker/task/10187?getfile=20104 posted http://www.rockbox.org/tracker/task/10187#comment31722 you’ll have it in the context menu, which i think is more convenient too.

would be be able to just hold select and get “delete theme” or would this be the only way of doing it this way?

Warning: Trying to access array offset on value of type null in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 648 Warning: Trying to access array offset on value of type null in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 650 Warning: Trying to access array offset on value of type null in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 648 Warning: Trying to access array offset on value of type null in /home/rockbox/flyspray/plugins/dokuwiki/inc/parser/xhtml.php on line 650

well now, last today i wrote that exactly. if you no know what i mean, no there is no other way of doing this.
you don’t know how compile?
you have vmwareplayer installed? can use vmware image debian.7z provided in wiki?
if yes login root. name: root password: rockbox
hit windows-e.
assume you winxp: go My Network Places.
if root at samba is shown double click
if not type \\debian\root into adress toolbar in current window and press enter
wait
asked for login again root/rockbox
save this page to \\debian\root name file: td.patch
http://www.rockbox.org/tracker/task/10187?getfile=20267 save this page to same dir name file: cm.patch
http://www.rockbox.org/tracker/task/10187?getfile=20104 click on vmware
right click open shells → eterm
execute command: vmware-toolbox& (note: sic!)
now you can copy text into bash. use ctrl-c and paste into shell by using shift+insert.
execute following command (assuming your player is gigabeat f):
cd && svn co svn://svn.rockbox.org/rockbox/branches/v3_3 rockbox && cd && patch -p0 < td.patch && patch -p0 < cm.patch && cd ~/rockbox && mkdir giga && cd giga && ../tools/configure –target=gigabeatf –type=n && make && make zip && mv rockbox.zip ~/gigabeat-f-patched-33.zip && rm -rf ~/rockbox

this command make you build with patch applied to 3.3 and context menu.

wow aparently i didnt see your previous comment. sorry

will this be committed some time soon?
the test builds have been up since about two weeks and there hasn’t been a single bug iiuc. the manual is ready and the functionality is given.

i think it would be simpler if it was in the context menu
i know you can patch it so its like that but will the final version be in the context menu? like theme_remove.3.patch

Sync, only offset errors. Second patch has context menu already included.

something went wrong with the buildzip.pl. don’t know what, but now it’s fixed.

teru commented on 2009-10-08 12:45

Update patch.

teru commented on 2009-10-11 14:12

Update patch.
* change string “Ask” to “Ask for Removal”.
* set default setting of font to “Ask for Removal”.
* Use splash function to show message.
* add some comments.

teru commented on 2009-10-12 14:55

update patch to add item to context menu.

TTL has expired on this task. see http://svn.rockbox.org/viewvc.cgi?view=rev;revision=23130 . please close it, so it won’t get listed in the “open bugs” section on the ml any more.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing